All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
@ 2014-11-18 18:54 ` Joe Stringer
  0 siblings, 0 replies; 11+ messages in thread
From: Joe Stringer @ 2014-11-18 18:54 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, pshelar, dev

When userspace doesn't provide a mask, OVS datapath generates a fully
unwildcarded mask for the flow. This is done by taking a copy of the
flow key, then iterating across its attributes, setting all values to
0xff. This works for most attributes, as the length of the netlink
attribute typically matches the length of the value. However, IPv6
labels only use the lower 20 bits of the field. This patch makes a
special case to handle this.

This fixes the following error seen when installing IPv6 flows without a mask:

openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 net/openvswitch/flow_netlink.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fa4ec2e..7a5b28f 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -825,7 +825,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 	return 0;
 }
 
-static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
+static void mask_set_nlattr(struct nlattr *attr)
 {
 	struct nlattr *nla;
 	int rem;
@@ -835,16 +835,18 @@ static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
 		/* We assume that ovs_key_lens[type] == -1 means that type is a
 		 * nested attribute
 		 */
-		if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
-			nlattr_set(nla, val, false);
+		if (ovs_key_lens[nla_type(nla)] == -1)
+			nla_for_each_nested(nla, attr, rem)
+				memset(nla_data(nla), 0xff, nla_len(nla));
 		else
-			memset(nla_data(nla), val, nla_len(nla));
-	}
-}
+			memset(nla_data(nla), 0xff, nla_len(nla));
 
-static void mask_set_nlattr(struct nlattr *attr, u8 val)
-{
-	nlattr_set(attr, val, true);
+		if (nla_type(nla) == OVS_KEY_ATTR_IPV6) {
+			struct ovs_key_ipv6 *ipv6_key = nla_data(nla);
+
+			ipv6_key->ipv6_label &= htonl(0x000FFFFF);
+		}
+	}
 }
 
 /**
@@ -926,7 +928,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 		if (!newmask)
 			return -ENOMEM;
 
-		mask_set_nlattr(newmask, 0xff);
+		mask_set_nlattr(newmask);
 
 		/* The userspace does not send tunnel attributes that are 0,
 		 * but we should not wildcard them nonetheless.
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
@ 2014-11-18 18:54 ` Joe Stringer
  0 siblings, 0 replies; 11+ messages in thread
From: Joe Stringer @ 2014-11-18 18:54 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA

When userspace doesn't provide a mask, OVS datapath generates a fully
unwildcarded mask for the flow. This is done by taking a copy of the
flow key, then iterating across its attributes, setting all values to
0xff. This works for most attributes, as the length of the netlink
attribute typically matches the length of the value. However, IPv6
labels only use the lower 20 bits of the field. This patch makes a
special case to handle this.

This fixes the following error seen when installing IPv6 flows without a mask:

openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 net/openvswitch/flow_netlink.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fa4ec2e..7a5b28f 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -825,7 +825,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 	return 0;
 }
 
-static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
+static void mask_set_nlattr(struct nlattr *attr)
 {
 	struct nlattr *nla;
 	int rem;
@@ -835,16 +835,18 @@ static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
 		/* We assume that ovs_key_lens[type] == -1 means that type is a
 		 * nested attribute
 		 */
-		if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
-			nlattr_set(nla, val, false);
+		if (ovs_key_lens[nla_type(nla)] == -1)
+			nla_for_each_nested(nla, attr, rem)
+				memset(nla_data(nla), 0xff, nla_len(nla));
 		else
-			memset(nla_data(nla), val, nla_len(nla));
-	}
-}
+			memset(nla_data(nla), 0xff, nla_len(nla));
 
-static void mask_set_nlattr(struct nlattr *attr, u8 val)
-{
-	nlattr_set(attr, val, true);
+		if (nla_type(nla) == OVS_KEY_ATTR_IPV6) {
+			struct ovs_key_ipv6 *ipv6_key = nla_data(nla);
+
+			ipv6_key->ipv6_label &= htonl(0x000FFFFF);
+		}
+	}
 }
 
 /**
@@ -926,7 +928,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 		if (!newmask)
 			return -ENOMEM;
 
-		mask_set_nlattr(newmask, 0xff);
+		mask_set_nlattr(newmask);
 
 		/* The userspace does not send tunnel attributes that are 0,
 		 * but we should not wildcard them nonetheless.
-- 
1.7.10.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-18 18:54 ` Joe Stringer
  (?)
@ 2014-11-19  6:09 ` Pravin Shelar
       [not found]   ` <CALnjE+qrkw92tZ-wgpGOPtbuMJVVCfo--7w6PAGO6OW4skiGQQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  -1 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2014-11-19  6:09 UTC (permalink / raw)
  To: Joe Stringer; +Cc: netdev, LKML, dev

On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com> wrote:
> When userspace doesn't provide a mask, OVS datapath generates a fully
> unwildcarded mask for the flow. This is done by taking a copy of the
> flow key, then iterating across its attributes, setting all values to
> 0xff. This works for most attributes, as the length of the netlink
> attribute typically matches the length of the value. However, IPv6
> labels only use the lower 20 bits of the field. This patch makes a
> special case to handle this.
>
> This fixes the following error seen when installing IPv6 flows without a mask:
>
> openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)
>
We should allow exact match mask here rather than generating
wildcarded mask. So that ovs can catch invalid ipv6.label.


> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  net/openvswitch/flow_netlink.c |   22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index fa4ec2e..7a5b28f 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -825,7 +825,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>         return 0;
>  }
>
> -static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
> +static void mask_set_nlattr(struct nlattr *attr)
>  {
>         struct nlattr *nla;
>         int rem;
> @@ -835,16 +835,18 @@ static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
>                 /* We assume that ovs_key_lens[type] == -1 means that type is a
>                  * nested attribute
>                  */
> -               if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
> -                       nlattr_set(nla, val, false);
> +               if (ovs_key_lens[nla_type(nla)] == -1)
> +                       nla_for_each_nested(nla, attr, rem)
> +                               memset(nla_data(nla), 0xff, nla_len(nla));
>                 else
> -                       memset(nla_data(nla), val, nla_len(nla));
> -       }
> -}
> +                       memset(nla_data(nla), 0xff, nla_len(nla));
>
> -static void mask_set_nlattr(struct nlattr *attr, u8 val)
> -{
> -       nlattr_set(attr, val, true);
> +               if (nla_type(nla) == OVS_KEY_ATTR_IPV6) {
> +                       struct ovs_key_ipv6 *ipv6_key = nla_data(nla);
> +
> +                       ipv6_key->ipv6_label &= htonl(0x000FFFFF);
> +               }
> +       }
>  }
>
>  /**
> @@ -926,7 +928,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>                 if (!newmask)
>                         return -ENOMEM;
>
> -               mask_set_nlattr(newmask, 0xff);
> +               mask_set_nlattr(newmask);
>
>                 /* The userspace does not send tunnel attributes that are 0,
>                  * but we should not wildcard them nonetheless.
> --
> 1.7.10.4
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
       [not found]   ` <CALnjE+qrkw92tZ-wgpGOPtbuMJVVCfo--7w6PAGO6OW4skiGQQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2014-11-19  7:25     ` Joe Stringer
  2014-11-19  8:11       ` [ovs-dev] " Pravin Shelar
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Stringer @ 2014-11-19  7:25 UTC (permalink / raw)
  To: Pravin Shelar; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev, LKML

On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:

> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
> wrote:
> > When userspace doesn't provide a mask, OVS datapath generates a fully
> > unwildcarded mask for the flow. This is done by taking a copy of the
> > flow key, then iterating across its attributes, setting all values to
> > 0xff. This works for most attributes, as the length of the netlink
> > attribute typically matches the length of the value. However, IPv6
> > labels only use the lower 20 bits of the field. This patch makes a
> > special case to handle this.
> >
> > This fixes the following error seen when installing IPv6 flows without a
> mask:
> >
> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
> max=fffff)
> >
> We should allow exact match mask here rather than generating
> wildcarded mask. So that ovs can catch invalid ipv6.label.


I don't quite follow, I thought this was exact-match? (The existing
function sets all bits to 1)

In this case, userspace has not specified a mask, but the kernel complains
about a mask that is too wide (because it generated a mask that's too
wide). Do you have an alternative fix in mind?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19  7:25     ` Joe Stringer
@ 2014-11-19  8:11       ` Pravin Shelar
  2014-11-19 17:48         ` Joe Stringer
  0 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2014-11-19  8:11 UTC (permalink / raw)
  To: Joe Stringer; +Cc: dev, netdev, LKML

On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
>>
>> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
>> wrote:
>> > When userspace doesn't provide a mask, OVS datapath generates a fully
>> > unwildcarded mask for the flow. This is done by taking a copy of the
>> > flow key, then iterating across its attributes, setting all values to
>> > 0xff. This works for most attributes, as the length of the netlink
>> > attribute typically matches the length of the value. However, IPv6
>> > labels only use the lower 20 bits of the field. This patch makes a
>> > special case to handle this.
>> >
>> > This fixes the following error seen when installing IPv6 flows without a
>> > mask:
>> >
>> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
>> > max=fffff)
>> >
>> We should allow exact match mask here rather than generating
>> wildcarded mask. So that ovs can catch invalid ipv6.label.
>
>
> I don't quite follow, I thought this was exact-match? (The existing function
> sets all bits to 1)
>
With 0xffffffff value we can exact match on all ipv6.lable bits.

> In this case, userspace has not specified a mask, but the kernel complains
> about a mask that is too wide (because it generated a mask that's too wide).
> Do you have an alternative fix in mind?

We can avoid the sanity check ipv6.lable for mask key.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19  8:11       ` [ovs-dev] " Pravin Shelar
@ 2014-11-19 17:48         ` Joe Stringer
  2014-11-19 19:08           ` Pravin Shelar
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Stringer @ 2014-11-19 17:48 UTC (permalink / raw)
  To: Pravin Shelar; +Cc: dev, netdev, LKML

On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com> 
wrote:
> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
> >> 
> >> wrote:
> >> > When userspace doesn't provide a mask, OVS datapath generates a fully
> >> > unwildcarded mask for the flow. This is done by taking a copy of the
> >> > flow key, then iterating across its attributes, setting all values to
> >> > 0xff. This works for most attributes, as the length of the netlink
> >> > attribute typically matches the length of the value. However, IPv6
> >> > labels only use the lower 20 bits of the field. This patch makes a
> >> > special case to handle this.
> >> > 
> >> > This fixes the following error seen when installing IPv6 flows without
> >> > a mask:
> >> > 
> >> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
> >> > max=fffff)
> >> 
> >> We should allow exact match mask here rather than generating
> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
> > 
> > I don't quite follow, I thought this was exact-match? (The existing
> > function sets all bits to 1)
> 
> With 0xffffffff value we can exact match on all ipv6.lable bits.

The label field is only 20 bits. The other bits in the same word of the IPv6 
header are for version (fixed) and traffic class (handled separately). We don't 
do anything with the other bits.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19 17:48         ` Joe Stringer
@ 2014-11-19 19:08           ` Pravin Shelar
  2014-11-19 19:51             ` Joe Stringer
  0 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2014-11-19 19:08 UTC (permalink / raw)
  To: Joe Stringer; +Cc: dev, netdev, LKML

On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <joestringer@nicira.com> wrote:
> On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
>> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com>
> wrote:
>> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
>> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
>> >>
>> >> wrote:
>> >> > When userspace doesn't provide a mask, OVS datapath generates a fully
>> >> > unwildcarded mask for the flow. This is done by taking a copy of the
>> >> > flow key, then iterating across its attributes, setting all values to
>> >> > 0xff. This works for most attributes, as the length of the netlink
>> >> > attribute typically matches the length of the value. However, IPv6
>> >> > labels only use the lower 20 bits of the field. This patch makes a
>> >> > special case to handle this.
>> >> >
>> >> > This fixes the following error seen when installing IPv6 flows without
>> >> > a mask:
>> >> >
>> >> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
>> >> > max=fffff)
>> >>
>> >> We should allow exact match mask here rather than generating
>> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
>> >
>> > I don't quite follow, I thought this was exact-match? (The existing
>> > function sets all bits to 1)
>>
>> With 0xffffffff value we can exact match on all ipv6.lable bits.
>
> The label field is only 20 bits. The other bits in the same word of the IPv6
> header are for version (fixed) and traffic class (handled separately). We don't
> do anything with the other bits.

This is just to make sure that we do not use those field for any thing
else. Masking those extra bits can hide incorrect ipv6 key extraction.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19 19:08           ` Pravin Shelar
@ 2014-11-19 19:51             ` Joe Stringer
  2014-11-19 20:33               ` Pravin Shelar
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Stringer @ 2014-11-19 19:51 UTC (permalink / raw)
  To: Pravin Shelar; +Cc: dev, netdev, LKML

On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <joestringer@nicira.com> 
wrote:
> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com>
> > 
> > wrote:
> >> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
> >> >> <joestringer@nicira.com>
> >> >> 
> >> >> wrote:
> >> >> > When userspace doesn't provide a mask, OVS datapath generates a
> >> >> > fully unwildcarded mask for the flow. This is done by taking a
> >> >> > copy of the flow key, then iterating across its attributes,
> >> >> > setting all values to 0xff. This works for most attributes, as the
> >> >> > length of the netlink attribute typically matches the length of
> >> >> > the value. However, IPv6 labels only use the lower 20 bits of the
> >> >> > field. This patch makes a special case to handle this.
> >> >> > 
> >> >> > This fixes the following error seen when installing IPv6 flows
> >> >> > without a mask:
> >> >> > 
> >> >> > openvswitch: netlink: Invalid IPv6 flow label value
> >> >> > (value=ffffffff, max=fffff)
> >> >> 
> >> >> We should allow exact match mask here rather than generating
> >> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
> >> > 
> >> > I don't quite follow, I thought this was exact-match? (The existing
> >> > function sets all bits to 1)
> >> 
> >> With 0xffffffff value we can exact match on all ipv6.lable bits.
> > 
> > The label field is only 20 bits. The other bits in the same word of the
> > IPv6 header are for version (fixed) and traffic class (handled
> > separately). We don't do anything with the other bits.
> 
> This is just to make sure that we do not use those field for any thing
> else. Masking those extra bits can hide incorrect ipv6 key extraction.

Oh, I see. I meant something more like:

ipv6_key->ipv6_label &= htonl(0xFFF00000);
ipv6_key->ipv6_label |= htonl(0x000FFFFF);

(Which would propagate the invalid bits from the flow key, but actually produce 
an exact match).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-18 18:54 ` Joe Stringer
  (?)
  (?)
@ 2014-11-19 20:19 ` David Miller
  -1 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2014-11-19 20:19 UTC (permalink / raw)
  To: joestringer; +Cc: netdev, linux-kernel, pshelar, dev

From: Joe Stringer <joestringer@nicira.com>
Date: Tue, 18 Nov 2014 10:54:17 -0800

> When userspace doesn't provide a mask, OVS datapath generates a fully
> unwildcarded mask for the flow. This is done by taking a copy of the
> flow key, then iterating across its attributes, setting all values to
> 0xff. This works for most attributes, as the length of the netlink
> attribute typically matches the length of the value. However, IPv6
> labels only use the lower 20 bits of the field. This patch makes a
> special case to handle this.
> 
> This fixes the following error seen when installing IPv6 flows without a mask:
> 
> openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Judging by the discussion ongoing about this patch, I am assuming there
will be a new version of this change forthcoming.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19 19:51             ` Joe Stringer
@ 2014-11-19 20:33               ` Pravin Shelar
  2014-11-19 20:49                 ` Joe Stringer
  0 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2014-11-19 20:33 UTC (permalink / raw)
  To: Joe Stringer; +Cc: dev, netdev, LKML

On Wed, Nov 19, 2014 at 11:51 AM, Joe Stringer <joestringer@nicira.com> wrote:
> On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
>> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <joestringer@nicira.com>
> wrote:
>> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
>> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com>
>> >
>> > wrote:
>> >> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
>> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
>> >> >> <joestringer@nicira.com>
>> >> >>
>> >> >> wrote:
>> >> >> > When userspace doesn't provide a mask, OVS datapath generates a
>> >> >> > fully unwildcarded mask for the flow. This is done by taking a
>> >> >> > copy of the flow key, then iterating across its attributes,
>> >> >> > setting all values to 0xff. This works for most attributes, as the
>> >> >> > length of the netlink attribute typically matches the length of
>> >> >> > the value. However, IPv6 labels only use the lower 20 bits of the
>> >> >> > field. This patch makes a special case to handle this.
>> >> >> >
>> >> >> > This fixes the following error seen when installing IPv6 flows
>> >> >> > without a mask:
>> >> >> >
>> >> >> > openvswitch: netlink: Invalid IPv6 flow label value
>> >> >> > (value=ffffffff, max=fffff)
>> >> >>
>> >> >> We should allow exact match mask here rather than generating
>> >> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
>> >> >
>> >> > I don't quite follow, I thought this was exact-match? (The existing
>> >> > function sets all bits to 1)
>> >>
>> >> With 0xffffffff value we can exact match on all ipv6.lable bits.
>> >
>> > The label field is only 20 bits. The other bits in the same word of the
>> > IPv6 header are for version (fixed) and traffic class (handled
>> > separately). We don't do anything with the other bits.
>>
>> This is just to make sure that we do not use those field for any thing
>> else. Masking those extra bits can hide incorrect ipv6 key extraction.
>
> Oh, I see. I meant something more like:
>
> ipv6_key->ipv6_label &= htonl(0xFFF00000);
> ipv6_key->ipv6_label |= htonl(0x000FFFFF);
>
> (Which would propagate the invalid bits from the flow key, but actually produce
> an exact match).

yes, it can wildcard unused bits.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
  2014-11-19 20:33               ` Pravin Shelar
@ 2014-11-19 20:49                 ` Joe Stringer
  0 siblings, 0 replies; 11+ messages in thread
From: Joe Stringer @ 2014-11-19 20:49 UTC (permalink / raw)
  To: Pravin Shelar; +Cc: dev, netdev, LKML

On Wednesday, November 19, 2014 12:33:10 Pravin Shelar wrote:
> On Wed, Nov 19, 2014 at 11:51 AM, Joe Stringer <joestringer@nicira.com> 
wrote:
> > On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
> >> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <joestringer@nicira.com>
> > 
> > wrote:
> >> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> >> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer
> >> >> <joestringer@nicira.com>
> >> > 
> >> > wrote:
> >> >> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
> >> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
> >> >> >> <joestringer@nicira.com>
> >> >> >> 
> >> >> >> wrote:
> >> >> >> > When userspace doesn't provide a mask, OVS datapath generates a
> >> >> >> > fully unwildcarded mask for the flow. This is done by taking a
> >> >> >> > copy of the flow key, then iterating across its attributes,
> >> >> >> > setting all values to 0xff. This works for most attributes, as
> >> >> >> > the length of the netlink attribute typically matches the
> >> >> >> > length of the value. However, IPv6 labels only use the lower 20
> >> >> >> > bits of the field. This patch makes a special case to handle
> >> >> >> > this.
> >> >> >> > 
> >> >> >> > This fixes the following error seen when installing IPv6 flows
> >> >> >> > without a mask:
> >> >> >> > 
> >> >> >> > openvswitch: netlink: Invalid IPv6 flow label value
> >> >> >> > (value=ffffffff, max=fffff)
> >> >> >> 
> >> >> >> We should allow exact match mask here rather than generating
> >> >> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
> >> >> > 
> >> >> > I don't quite follow, I thought this was exact-match? (The existing
> >> >> > function sets all bits to 1)
> >> >> 
> >> >> With 0xffffffff value we can exact match on all ipv6.lable bits.
> >> > 
> >> > The label field is only 20 bits. The other bits in the same word of
> >> > the IPv6 header are for version (fixed) and traffic class (handled
> >> > separately). We don't do anything with the other bits.
> >> 
> >> This is just to make sure that we do not use those field for any thing
> >> else. Masking those extra bits can hide incorrect ipv6 key extraction.
> > 
> > Oh, I see. I meant something more like:
> > 
> > ipv6_key->ipv6_label &= htonl(0xFFF00000);
> > ipv6_key->ipv6_label |= htonl(0x000FFFFF);
> > 
> > (Which would propagate the invalid bits from the flow key, but actually
> > produce an exact match).
> 
> yes, it can wildcard unused bits.

I'll send a v2.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-11-19 21:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-18 18:54 [PATCH net] openvswitch: Fix mask generation for IPv6 labels Joe Stringer
2014-11-18 18:54 ` Joe Stringer
2014-11-19  6:09 ` Pravin Shelar
     [not found]   ` <CALnjE+qrkw92tZ-wgpGOPtbuMJVVCfo--7w6PAGO6OW4skiGQQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-11-19  7:25     ` Joe Stringer
2014-11-19  8:11       ` [ovs-dev] " Pravin Shelar
2014-11-19 17:48         ` Joe Stringer
2014-11-19 19:08           ` Pravin Shelar
2014-11-19 19:51             ` Joe Stringer
2014-11-19 20:33               ` Pravin Shelar
2014-11-19 20:49                 ` Joe Stringer
2014-11-19 20:19 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.