[ovs-dev,v4,2/2] conntrack: Exclude l2 padding in 'conn_key_extract()'.

Message ID 1549326187-82147-2-git-send-email-dlu998@gmail.com
State Changes Requested
Headers show
Series
  • [ovs-dev,v4,1/2] dp-packet: Add 'dp_packet_l3_size()'.
Related show

Commit Message

Darrell Ball Feb. 5, 2019, 12:23 a.m.
'conn_key_extract()' in userspace conntrack is including L2
(Ethernet) pad bytes for both L3 and L4 sizes. One problem is
any packet with non-zero L2 padding can incorrectly fail L4
checksum validation.

This patch fixes conn_key_extract() by ignoring L2 pad bytes.

Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
CC: Daniele Di Proietto <diproiettod@ovn.org>
Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Co-authored-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---

v3: Removed line inadvertently merged from another patch.

 lib/conntrack.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Ben Pfaff Feb. 12, 2019, 3:43 a.m. | #1
On Mon, Feb 04, 2019 at 04:23:07PM -0800, Darrell Ball wrote:
> 'conn_key_extract()' in userspace conntrack is including L2
> (Ethernet) pad bytes for both L3 and L4 sizes. One problem is
> any packet with non-zero L2 padding can incorrectly fail L4
> checksum validation.
> 
> This patch fixes conn_key_extract() by ignoring L2 pad bytes.
> 
> Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> CC: Daniele Di Proietto <diproiettod@ovn.org>
> Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> Co-authored-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
> Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> Signed-off-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
> Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>

Thanks.

I applied this series to master.  Please let me know what branches I
should backport it to.
Nitin Katiyar Feb. 13, 2019, 6:09 a.m. | #2
Hi,
We would like to get it backported till OVS 2.6 at least.

Regards,
Nitin

> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Tuesday, February 12, 2019 9:13 AM
> To: Darrell Ball <dlu998@gmail.com>
> Cc: dev@openvswitch.org; Nitin Katiyar <nitin.katiyar@ericsson.com>
> Subject: Re: [ovs-dev] [patch v4 2/2] conntrack: Exclude l2 padding in
> 'conn_key_extract()'.
> 
> On Mon, Feb 04, 2019 at 04:23:07PM -0800, Darrell Ball wrote:
> > 'conn_key_extract()' in userspace conntrack is including L2
> > (Ethernet) pad bytes for both L3 and L4 sizes. One problem is any
> > packet with non-zero L2 padding can incorrectly fail L4 checksum
> > validation.
> >
> > This patch fixes conn_key_extract() by ignoring L2 pad bytes.
> >
> > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> > CC: Daniele Di Proietto <diproiettod@ovn.org>
> > Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> > Co-authored-by: Venkatesan Pradeep
> <venkatesan.pradeep@ericsson.com>
> > Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> > Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> > Signed-off-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
> > Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> 
> Thanks.
> 
> I applied this series to master.  Please let me know what branches I should
> backport it to.
Ben Pfaff Feb. 13, 2019, 5:03 p.m. | #3
I backported as far as 2.9.  For 2.8, it didn't apply cleanly--please
post backports.

On Wed, Feb 13, 2019 at 06:09:03AM +0000, Nitin Katiyar wrote:
> Hi,
> We would like to get it backported till OVS 2.6 at least.
> 
> Regards,
> Nitin
> 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp@ovn.org]
> > Sent: Tuesday, February 12, 2019 9:13 AM
> > To: Darrell Ball <dlu998@gmail.com>
> > Cc: dev@openvswitch.org; Nitin Katiyar <nitin.katiyar@ericsson.com>
> > Subject: Re: [ovs-dev] [patch v4 2/2] conntrack: Exclude l2 padding in
> > 'conn_key_extract()'.
> > 
> > On Mon, Feb 04, 2019 at 04:23:07PM -0800, Darrell Ball wrote:
> > > 'conn_key_extract()' in userspace conntrack is including L2
> > > (Ethernet) pad bytes for both L3 and L4 sizes. One problem is any
> > > packet with non-zero L2 padding can incorrectly fail L4 checksum
> > > validation.
> > >
> > > This patch fixes conn_key_extract() by ignoring L2 pad bytes.
> > >
> > > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> > > CC: Daniele Di Proietto <diproiettod@ovn.org>
> > > Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> > > Co-authored-by: Venkatesan Pradeep
> > <venkatesan.pradeep@ericsson.com>
> > > Co-authored-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> > > Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> > > Signed-off-by: Venkatesan Pradeep <venkatesan.pradeep@ericsson.com>
> > > Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> > > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> > 
> > Thanks.
> > 
> > I applied this series to master.  Please let me know what branches I should
> > backport it to.

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 0fedddd..e1f4041 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1875,6 +1875,9 @@  extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
  * processed, the function will extract the key from the packet nested
  * in the ICMP payload and set '*related' to true.
  *
+ * 'size' here is the layer 4 size, which can be a nested size if parsing
+ * an ICMP or ICMP6 header.
+ *
  * If 'related' is NULL, it means that we're already parsing a header nested
  * in an ICMP error.  In this case, we skip checksum and length validation. */
 static inline bool
@@ -1949,7 +1952,6 @@  conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
      *     we use a sparse representation (miniflow).
      *
      */
-    const char *tail = dp_packet_tail(pkt);
     bool ok;
     ctx->key.dl_type = dl_type;
 
@@ -1960,11 +1962,11 @@  conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         } else {
             bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt);
             /* Validate the checksum only when hwol is not supported. */
-            ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
+            ok = extract_l3_ipv4(&ctx->key, l3, dp_packet_l3_size(pkt), NULL,
                                  !hwol_good_l3_csum);
         }
     } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-        ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL);
+        ok = extract_l3_ipv6(&ctx->key, l3, dp_packet_l3_size(pkt), NULL);
     } else {
         ok = false;
     }
@@ -1974,8 +1976,8 @@  conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         if (!hwol_bad_l4_csum) {
             bool  hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt);
             /* Validate the checksum only when hwol is not supported. */
-            if (extract_l4(&ctx->key, l4, tail - l4, &ctx->icmp_related, l3,
-                           !hwol_good_l4_csum)) {
+            if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt),
+                           &ctx->icmp_related, l3, !hwol_good_l4_csum)) {
                 ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
                 return true;
             }