>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. > Hi Ilya, it's OK and thanks for your review. >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. Will address this in v4 > >> + 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? Agree, will address this in v4 > >> + 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. good catch. Will do > >> + 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
