Re: [ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt
>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
>> ---
>> 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="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010500220005333"
>> +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1)
>> +pkt2="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010100220009333"
>> +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
Re: [ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt
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
> ---
> 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="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010500220005333"
> +dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1)
> +pkt2="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010100220009333"
> +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:
>
[ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt
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
---
lib/flow.c| 18 ++
tests/ofproto-dpif.at | 31 +++
2 files changed, 41 insertions(+), 8 deletions(-)
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;
+if (tcp_hdr_len >= TCP_HEADER_LEN) {
+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);
+}
}
} 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="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010500220005333"
+dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1)
+pkt2="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010100220009333"
+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
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
