On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <[email protected]> wrote:
> On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
>>
>> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <[email protected]>
>> 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.
On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <[email protected]>
wrote:
> > On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <[email protected]>
> >>
> >> 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.
On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <[email protected]> wrote:
> On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
>> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <[email protected]>
> wrote:
>> > On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
>> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <[email protected]>
>> >>
>> >> 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.
On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <[email protected]>
wrote:
> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <[email protected]>
> >
> > wrote:
> >> > On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
> >> >> <[email protected]>
> >> >>
> >> >> 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).
On Wed, Nov 19, 2014 at 11:51 AM, Joe Stringer <[email protected]> wrote:
> On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
>> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <[email protected]>
> wrote:
>> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
>> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <[email protected]>
>> >
>> > wrote:
>> >> > On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
>> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
>> >> >> <[email protected]>
>> >> >>
>> >> >> 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.
On Wednesday, November 19, 2014 12:33:10 Pravin Shelar wrote:
> On Wed, Nov 19, 2014 at 11:51 AM, Joe Stringer <[email protected]>
wrote:
> > On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
> >> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <[email protected]>
> >
> > wrote:
> >> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> >> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer
> >> >> <[email protected]>
> >> >
> >> > wrote:
> >> >> > On 18 November 2014 22:09, Pravin Shelar <[email protected]> wrote:
> >> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
> >> >> >> <[email protected]>
> >> >> >>
> >> >> >> 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.