On 11/1/21 12:03, lic121 wrote:
> dataofs field of tcp header indicates the tcp header len. The len
> should be >= 20 bytes/4. This patch is to test dataofs, and don't
> parse layer 4 fields when meet ba dataofs. This behave is the consistent
> with openvswitch kenrel module.
>
> Signed-off-by: lic121 <[email protected]>
> ---
> lib/flow.c | 18 ++++++++++--------
> tests/ofproto-dpif.at | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 8 deletions(-)
Hi. Thanks for the patch! And sorry for the late response.
See some comments inline.
Bets regards, Ilya Maximets.
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 89837de..f117490 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct
> miniflow *dst)
> if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
> if (OVS_LIKELY(size >= TCP_HEADER_LEN)) {
> const struct tcp_header *tcp = data;
> -
> - miniflow_push_be32(mf, arp_tha.ea[2], 0);
> - miniflow_push_be32(mf, tcp_flags,
> - TCP_FLAGS_BE32(tcp->tcp_ctl));
> - miniflow_push_be16(mf, tp_src, tcp->tcp_src);
> - miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
> - miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
> - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
> + size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
Please, add an empty line between the declarations and the code below.
> + if (tcp_hdr_len >= TCP_HEADER_LEN) {
This check seems fine, but, IIUC, it doesn't check the case where
the TCP header length declared larger than the remaining space in
a packet. Looking at the kernel implementation of tcphdr_ok(),
there are, basically, 3 checks that make it fail:
1. pskb_may_pull(skb, th_ofs + sizeof(struct tcphdr))) == false
This one is similar to (size >= TCP_HEADER_LEN) check here.
2. tcp_len < sizeof(struct tcphdr)
This check you added as (tcp_hdr_len >= TCP_HEADER_LEN)
3. skb->len < th_ofs + tcp_len
But this one is not covered by the patch. IIUC, it should
look like:
if (tcp_hdr_len >= TCP_HEADER_LEN && size >= tcp_hdr_len) {
Without the third check, if TCP header length is larger than
the remaining packet size, kernel will not parse it, but
userspace will, leading to the similar issue.
Could you add this check to the patch and a test case for this
condition?
> + miniflow_push_be32(mf, arp_tha.ea[2], 0);
> + miniflow_push_be32(mf, tcp_flags,
> + TCP_FLAGS_BE32(tcp->tcp_ctl));
Please, shift this line 3 spaces to the right.
> + miniflow_push_be16(mf, tp_src, tcp->tcp_src);
> + miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
> + miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
> + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
> + }
> }
> } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) {
> if (OVS_LIKELY(size >= UDP_HEADER_LEN)) {
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 31fb163..0f372ae 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -4862,6 +4862,37 @@
> recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - malformed packets handling - upcall])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 90
> +dnl drop packet has tcp port 0-f but allow other tcp packets
> +AT_DATA([flows.txt], [dnl
> +priority=75 tcp tp_dst=0/0xfff0 actions=drop
> +priority=50 tcp actions=output:1
> +])
> +AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
> +dnl good tcp pkt, tcp(sport=100,dpor=16)
> +pkt1="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000005002200053330000"
> +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1)
> +pkt2="be95df40fb57fa163e5ee3570800450000280001000040063e940a0a0a0a141414140064001000000000000000001002200093330000"
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> +mode=normal
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout])
> +dnl for good tcp pkt, ovs can extract the tp_dst=16
> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
> +flow-dump from the main thread:
> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0),
> packets:0, bytes:0, used:never, actions:1
> +])
> +AT_CHECK([ovs-appctl dpctl/del-flows], [0], [stdout])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout])
> +dnl for malformed tcp pkt, ovs can use default value tp_dst=0
> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
> +flow-dump from the main thread:
> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0),
> packets:0, bytes:0, used:never, actions:drop
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto-dpif - exit])
> OVS_VSWITCHD_START
> add_of_ports br0 1 2 3 10 11 12 13 14
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
