[ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt

[lic121]

>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 <lic121 at chinatelecom.cn>



>> ---



>>  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



>> 



>



>