| Message ID | 20220309222033.3018976-1-i.maximets@ovn.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1926407a4ab0e59d5a27bed7b82029b356d80fa0 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: openvswitch: fix uAPI incompatibility with existing user space | expand |
Le 09/03/2022 à 23:20, Ilya Maximets a écrit : > Few years ago OVS user space made a strange choice in the commit [1] > to define types only valid for the user space inside the copy of a > kernel uAPI header. '#ifndef __KERNEL__' and another attribute was > added later. > > This leads to the inevitable clash between user space and kernel types > when the kernel uAPI is extended. The issue was unveiled with the > addition of a new type for IPv6 extension header in kernel uAPI. > > When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the > older user space application, application tries to parse it as > OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as > malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with > every IPv6 packet that goes to the user space, IPv6 support is fully > broken. > > Fixing that by bringing these user space attributes to the kernel > uAPI to avoid the clash. Strictly speaking this is not the problem > of the kernel uAPI, but changing it is the only way to avoid breakage > of the older user space applications at this point. > > These 2 types are explicitly rejected now since they should not be > passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved > out from the '#ifdef __KERNEL__' as there is no good reason to hide > it from the userspace. And it's also explicitly rejected now, because > it's for in-kernel use only. > > Comments with warnings were added to avoid the problem coming back. > > (1 << type) converted to (1ULL << type) to avoid integer overflow on > OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. > > [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") > > Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") > Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com > Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c > Reported-by: Roi Dayan <roid@nvidia.com> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Thanks for finally doing this. I also suggest it months ago: https://lore.kernel.org/lkml/a4894aef-b82a-8224-611d-07be229f5ebe@6wind.com/ Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Ilya Maximets <i.maximets@ovn.org> writes: > Few years ago OVS user space made a strange choice in the commit [1] > to define types only valid for the user space inside the copy of a > kernel uAPI header. '#ifndef __KERNEL__' and another attribute was > added later. > > This leads to the inevitable clash between user space and kernel types > when the kernel uAPI is extended. The issue was unveiled with the > addition of a new type for IPv6 extension header in kernel uAPI. > > When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the > older user space application, application tries to parse it as > OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as > malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with > every IPv6 packet that goes to the user space, IPv6 support is fully > broken. > > Fixing that by bringing these user space attributes to the kernel > uAPI to avoid the clash. Strictly speaking this is not the problem > of the kernel uAPI, but changing it is the only way to avoid breakage > of the older user space applications at this point. > > These 2 types are explicitly rejected now since they should not be > passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved > out from the '#ifdef __KERNEL__' as there is no good reason to hide > it from the userspace. And it's also explicitly rejected now, because > it's for in-kernel use only. > > Comments with warnings were added to avoid the problem coming back. > > (1 << type) converted to (1ULL << type) to avoid integer overflow on > OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. > > [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") > > Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") > Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com > Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c > Reported-by: Roi Dayan <roid@nvidia.com> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Acked-by: Aaron Conole <aconole@redhat.com>
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 9 Mar 2022 23:20:33 +0100 you wrote: > Few years ago OVS user space made a strange choice in the commit [1] > to define types only valid for the user space inside the copy of a > kernel uAPI header. '#ifndef __KERNEL__' and another attribute was > added later. > > This leads to the inevitable clash between user space and kernel types > when the kernel uAPI is extended. The issue was unveiled with the > addition of a new type for IPv6 extension header in kernel uAPI. > > [...] Here is the summary with links: - [net-next,v2] net: openvswitch: fix uAPI incompatibility with existing user space https://git.kernel.org/netdev/net-next/c/1926407a4ab0 You are awesome, thank you!
On 2022-03-10 8:44 PM, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> Few years ago OVS user space made a strange choice in the commit [1] >> to define types only valid for the user space inside the copy of a >> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >> added later. >> >> This leads to the inevitable clash between user space and kernel types >> when the kernel uAPI is extended. The issue was unveiled with the >> addition of a new type for IPv6 extension header in kernel uAPI. >> >> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >> older user space application, application tries to parse it as >> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >> every IPv6 packet that goes to the user space, IPv6 support is fully >> broken. >> >> Fixing that by bringing these user space attributes to the kernel >> uAPI to avoid the clash. Strictly speaking this is not the problem >> of the kernel uAPI, but changing it is the only way to avoid breakage >> of the older user space applications at this point. >> >> These 2 types are explicitly rejected now since they should not be >> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >> out from the '#ifdef __KERNEL__' as there is no good reason to hide >> it from the userspace. And it's also explicitly rejected now, because >> it's for in-kernel use only. >> >> Comments with warnings were added to avoid the problem coming back. >> >> (1 << type) converted to (1ULL << type) to avoid integer overflow on >> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >> >> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >> >> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >> Reported-by: Roi Dayan <roid@nvidia.com> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > Acked-by: Aaron Conole <aconole@redhat.com> > I got to check traffic with the fix and I do get some traffic but something is broken. I didn't investigate much but the quick test shows me rules are not offloaded and dumping ovs rules gives error like this recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2)
On 3/14/22 19:33, Roi Dayan wrote: > > > On 2022-03-10 8:44 PM, Aaron Conole wrote: >> Ilya Maximets <i.maximets@ovn.org> writes: >> >>> Few years ago OVS user space made a strange choice in the commit [1] >>> to define types only valid for the user space inside the copy of a >>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>> added later. >>> >>> This leads to the inevitable clash between user space and kernel types >>> when the kernel uAPI is extended. The issue was unveiled with the >>> addition of a new type for IPv6 extension header in kernel uAPI. >>> >>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>> older user space application, application tries to parse it as >>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>> every IPv6 packet that goes to the user space, IPv6 support is fully >>> broken. >>> >>> Fixing that by bringing these user space attributes to the kernel >>> uAPI to avoid the clash. Strictly speaking this is not the problem >>> of the kernel uAPI, but changing it is the only way to avoid breakage >>> of the older user space applications at this point. >>> >>> These 2 types are explicitly rejected now since they should not be >>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>> it from the userspace. And it's also explicitly rejected now, because >>> it's for in-kernel use only. >>> >>> Comments with warnings were added to avoid the problem coming back. >>> >>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>> >>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>> >>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>> Reported-by: Roi Dayan <roid@nvidia.com> >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >> >> Acked-by: Aaron Conole <aconole@redhat.com> >> > > > > I got to check traffic with the fix and I do get some traffic > but something is broken. I didn't investigate much but the quick > test shows me rules are not offloaded and dumping ovs rules gives > error like this > > recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) Such a dump is expected, because kernel parses fields that current userspace doesn't understand, and at the same time OVS by design is using kernel provided key/mask while installing datapath rules, IIRC. It should be possible to make these dumps a bit more friendly though. For the offloading not working, see my comment in the v2 patch email I sent (top email of this thread). In short, it's a problem in user space and it can not be fixed from the kernel side, unless we revert IPv6 extension header support and never add any new types, which is unreasonable. I didn't test any actual offloading, but I had a successful run of 'make check-offloads' with my quick'n'dirty fix from the top email. Since we're here: Toms, do you plan to submit user space patches for this feature? Best regards, Ilya Maximets.
On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: > On 3/14/22 19:33, Roi Dayan wrote: >> >> >> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>> Ilya Maximets <i.maximets@ovn.org> writes: >>> >>>> Few years ago OVS user space made a strange choice in the commit [1] >>>> to define types only valid for the user space inside the copy of a >>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>> added later. >>>> >>>> This leads to the inevitable clash between user space and kernel types >>>> when the kernel uAPI is extended. The issue was unveiled with the >>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>> >>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>> older user space application, application tries to parse it as >>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>> broken. >>>> >>>> Fixing that by bringing these user space attributes to the kernel >>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>> of the older user space applications at this point. >>>> >>>> These 2 types are explicitly rejected now since they should not be >>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>> it from the userspace. And it's also explicitly rejected now, because >>>> it's for in-kernel use only. >>>> >>>> Comments with warnings were added to avoid the problem coming back. >>>> >>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>> >>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>> >>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> --- >>> >>> Acked-by: Aaron Conole <aconole@redhat.com> >>> >> >> >> >> I got to check traffic with the fix and I do get some traffic >> but something is broken. I didn't investigate much but the quick >> test shows me rules are not offloaded and dumping ovs rules gives >> error like this >> >> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) > > Such a dump is expected, because kernel parses fields that current > userspace doesn't understand, and at the same time OVS by design is > using kernel provided key/mask while installing datapath rules, IIRC. > It should be possible to make these dumps a bit more friendly though. > > For the offloading not working, see my comment in the v2 patch email > I sent (top email of this thread). In short, it's a problem in user > space and it can not be fixed from the kernel side, unless we revert > IPv6 extension header support and never add any new types, which is > unreasonable. I didn't test any actual offloading, but I had a > successful run of 'make check-offloads' with my quick'n'dirty fix from > the top email. Hi Ilya, I can confirm that with latest OvS master IPv6 rules offload still fails without your pastebin code applied. > > Since we're here: > > Toms, do you plan to submit user space patches for this feature? I see there is a patch from you that is supposed to fix compatibility issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align uAPI definition with the kernel."), but it doesn't fix offload for me without pastebin patch. Do you plan to merge that code into OvS or you require some help from our side? Regards, Vlad [...]
On 4/7/22 10:02, Vlad Buslov wrote: > On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >> On 3/14/22 19:33, Roi Dayan wrote: >>> >>> >>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>> >>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>> to define types only valid for the user space inside the copy of a >>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>> added later. >>>>> >>>>> This leads to the inevitable clash between user space and kernel types >>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>> >>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>> older user space application, application tries to parse it as >>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>> broken. >>>>> >>>>> Fixing that by bringing these user space attributes to the kernel >>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>> of the older user space applications at this point. >>>>> >>>>> These 2 types are explicitly rejected now since they should not be >>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>> it from the userspace. And it's also explicitly rejected now, because >>>>> it's for in-kernel use only. >>>>> >>>>> Comments with warnings were added to avoid the problem coming back. >>>>> >>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>> >>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>> >>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>> --- >>>> >>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>> >>> >>> >>> >>> I got to check traffic with the fix and I do get some traffic >>> but something is broken. I didn't investigate much but the quick >>> test shows me rules are not offloaded and dumping ovs rules gives >>> error like this >>> >>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >> >> Such a dump is expected, because kernel parses fields that current >> userspace doesn't understand, and at the same time OVS by design is >> using kernel provided key/mask while installing datapath rules, IIRC. >> It should be possible to make these dumps a bit more friendly though. >> >> For the offloading not working, see my comment in the v2 patch email >> I sent (top email of this thread). In short, it's a problem in user >> space and it can not be fixed from the kernel side, unless we revert >> IPv6 extension header support and never add any new types, which is >> unreasonable. I didn't test any actual offloading, but I had a >> successful run of 'make check-offloads' with my quick'n'dirty fix from >> the top email. > > Hi Ilya, > > I can confirm that with latest OvS master IPv6 rules offload still fails > without your pastebin code applied. > >> >> Since we're here: >> >> Toms, do you plan to submit user space patches for this feature? > > I see there is a patch from you that is supposed to fix compatibility > issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align > uAPI definition with the kernel."), but it doesn't fix offload for me > without pastebin patch. Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. Issue with offload is an OVS bug that should be fixed separately. The fix will also need to be backported to OVS stable branches. > Do you plan to merge that code into OvS or you > require some help from our side? I could do that, but I don't really have enough time. So, if you can work on that fix, it would be great. Note that comments inside the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly copied from the userspace datapath and are incorrect for the general case, so has to be fixed alongside the logic of that function. Best regards, Ilya Maximets.
On Thu 12 May 2022 at 12:19, Eelco Chaudron <echaudro@redhat.com> wrote: > On 7 Apr 2022, at 12:22, Ilya Maximets wrote: > >> On 4/7/22 10:02, Vlad Buslov wrote: >>> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >>>> On 3/14/22 19:33, Roi Dayan wrote: >>>>> >>>>> >>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>>> >>>>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>>>> to define types only valid for the user space inside the copy of a >>>>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>>>> added later. >>>>>>> >>>>>>> This leads to the inevitable clash between user space and kernel types >>>>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>>>> >>>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>>>> older user space application, application tries to parse it as >>>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>>>> broken. >>>>>>> >>>>>>> Fixing that by bringing these user space attributes to the kernel >>>>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>>>> of the older user space applications at this point. >>>>>>> >>>>>>> These 2 types are explicitly rejected now since they should not be >>>>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>>>> it from the userspace. And it's also explicitly rejected now, because >>>>>>> it's for in-kernel use only. >>>>>>> >>>>>>> Comments with warnings were added to avoid the problem coming back. >>>>>>> >>>>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>>>> >>>>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>>>> >>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>>> --- >>>>>> >>>>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>>>> >>>>> >>>>> >>>>> >>>>> I got to check traffic with the fix and I do get some traffic >>>>> but something is broken. I didn't investigate much but the quick >>>>> test shows me rules are not offloaded and dumping ovs rules gives >>>>> error like this >>>>> >>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>>> >>>> Such a dump is expected, because kernel parses fields that current >>>> userspace doesn't understand, and at the same time OVS by design is >>>> using kernel provided key/mask while installing datapath rules, IIRC. >>>> It should be possible to make these dumps a bit more friendly though. >>>> >>>> For the offloading not working, see my comment in the v2 patch email >>>> I sent (top email of this thread). In short, it's a problem in user >>>> space and it can not be fixed from the kernel side, unless we revert >>>> IPv6 extension header support and never add any new types, which is >>>> unreasonable. I didn't test any actual offloading, but I had a >>>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>>> the top email. >>> >>> Hi Ilya, >>> >>> I can confirm that with latest OvS master IPv6 rules offload still fails >>> without your pastebin code applied. >>> >>>> >>>> Since we're here: >>>> >>>> Toms, do you plan to submit user space patches for this feature? >>> >>> I see there is a patch from you that is supposed to fix compatibility >>> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >>> uAPI definition with the kernel."), but it doesn't fix offload for me >>> without pastebin patch. >> >> Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. >> Issue with offload is an OVS bug that should be fixed separately. >> The fix will also need to be backported to OVS stable branches. >> >>> Do you plan to merge that code into OvS or you >>> require some help from our side? >> >> I could do that, but I don't really have enough time. So, if you >> can work on that fix, it would be great. Note that comments inside >> the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly >> copied from the userspace datapath and are incorrect for the general >> case, so has to be fixed alongside the logic of that function. > > Tom or Vlad, are you working on this? Asking, as the release of a kernel with > Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will > break OVS. > > //Eelco Hi Eelco, My simple fix for OvS was rejected and I don't have time to rework it at the moment. Regards, Vlad
On 7 Apr 2022, at 12:22, Ilya Maximets wrote: > On 4/7/22 10:02, Vlad Buslov wrote: >> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >>> On 3/14/22 19:33, Roi Dayan wrote: >>>> >>>> >>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>> >>>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>>> to define types only valid for the user space inside the copy of a >>>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>>> added later. >>>>>> >>>>>> This leads to the inevitable clash between user space and kernel types >>>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>>> >>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>>> older user space application, application tries to parse it as >>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>>> broken. >>>>>> >>>>>> Fixing that by bringing these user space attributes to the kernel >>>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>>> of the older user space applications at this point. >>>>>> >>>>>> These 2 types are explicitly rejected now since they should not be >>>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>>> it from the userspace. And it's also explicitly rejected now, because >>>>>> it's for in-kernel use only. >>>>>> >>>>>> Comments with warnings were added to avoid the problem coming back. >>>>>> >>>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>>> >>>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>>> >>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>> --- >>>>> >>>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>>> >>>> >>>> >>>> >>>> I got to check traffic with the fix and I do get some traffic >>>> but something is broken. I didn't investigate much but the quick >>>> test shows me rules are not offloaded and dumping ovs rules gives >>>> error like this >>>> >>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>> >>> Such a dump is expected, because kernel parses fields that current >>> userspace doesn't understand, and at the same time OVS by design is >>> using kernel provided key/mask while installing datapath rules, IIRC. >>> It should be possible to make these dumps a bit more friendly though. >>> >>> For the offloading not working, see my comment in the v2 patch email >>> I sent (top email of this thread). In short, it's a problem in user >>> space and it can not be fixed from the kernel side, unless we revert >>> IPv6 extension header support and never add any new types, which is >>> unreasonable. I didn't test any actual offloading, but I had a >>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>> the top email. >> >> Hi Ilya, >> >> I can confirm that with latest OvS master IPv6 rules offload still fails >> without your pastebin code applied. >> >>> >>> Since we're here: >>> >>> Toms, do you plan to submit user space patches for this feature? >> >> I see there is a patch from you that is supposed to fix compatibility >> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >> uAPI definition with the kernel."), but it doesn't fix offload for me >> without pastebin patch. > > Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. > Issue with offload is an OVS bug that should be fixed separately. > The fix will also need to be backported to OVS stable branches. > >> Do you plan to merge that code into OvS or you >> require some help from our side? > > I could do that, but I don't really have enough time. So, if you > can work on that fix, it would be great. Note that comments inside > the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly > copied from the userspace datapath and are incorrect for the general > case, so has to be fixed alongside the logic of that function. Tom or Vlad, are you working on this? Asking, as the release of a kernel with Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will break OVS. //Eelco
On 12 May 2022, at 12:08, Vlad Buslov wrote: > On Thu 12 May 2022 at 12:19, Eelco Chaudron <echaudro@redhat.com> wrote: >> On 7 Apr 2022, at 12:22, Ilya Maximets wrote: >> >>> On 4/7/22 10:02, Vlad Buslov wrote: >>>> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >>>>> On 3/14/22 19:33, Roi Dayan wrote: >>>>>> >>>>>> >>>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>>>> >>>>>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>>>>> to define types only valid for the user space inside the copy of a >>>>>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>>>>> added later. >>>>>>>> >>>>>>>> This leads to the inevitable clash between user space and kernel types >>>>>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>>>>> >>>>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>>>>> older user space application, application tries to parse it as >>>>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>>>>> broken. >>>>>>>> >>>>>>>> Fixing that by bringing these user space attributes to the kernel >>>>>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>>>>> of the older user space applications at this point. >>>>>>>> >>>>>>>> These 2 types are explicitly rejected now since they should not be >>>>>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>>>>> it from the userspace. And it's also explicitly rejected now, because >>>>>>>> it's for in-kernel use only. >>>>>>>> >>>>>>>> Comments with warnings were added to avoid the problem coming back. >>>>>>>> >>>>>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>>>>> >>>>>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>>>>> >>>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>>>> --- >>>>>>> >>>>>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I got to check traffic with the fix and I do get some traffic >>>>>> but something is broken. I didn't investigate much but the quick >>>>>> test shows me rules are not offloaded and dumping ovs rules gives >>>>>> error like this >>>>>> >>>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>>>> >>>>> Such a dump is expected, because kernel parses fields that current >>>>> userspace doesn't understand, and at the same time OVS by design is >>>>> using kernel provided key/mask while installing datapath rules, IIRC. >>>>> It should be possible to make these dumps a bit more friendly though. >>>>> >>>>> For the offloading not working, see my comment in the v2 patch email >>>>> I sent (top email of this thread). In short, it's a problem in user >>>>> space and it can not be fixed from the kernel side, unless we revert >>>>> IPv6 extension header support and never add any new types, which is >>>>> unreasonable. I didn't test any actual offloading, but I had a >>>>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>>>> the top email. >>>> >>>> Hi Ilya, >>>> >>>> I can confirm that with latest OvS master IPv6 rules offload still fails >>>> without your pastebin code applied. >>>> >>>>> >>>>> Since we're here: >>>>> >>>>> Toms, do you plan to submit user space patches for this feature? >>>> >>>> I see there is a patch from you that is supposed to fix compatibility >>>> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >>>> uAPI definition with the kernel."), but it doesn't fix offload for me >>>> without pastebin patch. >>> >>> Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. >>> Issue with offload is an OVS bug that should be fixed separately. >>> The fix will also need to be backported to OVS stable branches. >>> >>>> Do you plan to merge that code into OvS or you >>>> require some help from our side? >>> >>> I could do that, but I don't really have enough time. So, if you >>> can work on that fix, it would be great. Note that comments inside >>> the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly >>> copied from the userspace datapath and are incorrect for the general >>> case, so has to be fixed alongside the logic of that function. >> >> Tom or Vlad, are you working on this? Asking, as the release of a kernel with >> Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will >> break OVS. >> >> //Eelco > > Hi Eelco, > > My simple fix for OvS was rejected and I don't have time to rework it at > the moment. That’s a pity, Tom do you maybe have time as your patch left OVS in this error state? //Eelco
On 17 May 2022, at 13:10, Eelco Chaudron wrote: > On 12 May 2022, at 12:08, Vlad Buslov wrote: > >> On Thu 12 May 2022 at 12:19, Eelco Chaudron <echaudro@redhat.com> wrote: >>> On 7 Apr 2022, at 12:22, Ilya Maximets wrote: >>> >>>> On 4/7/22 10:02, Vlad Buslov wrote: >>>>> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >>>>>> On 3/14/22 19:33, Roi Dayan wrote: >>>>>>> >>>>>>> >>>>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>>>>> >>>>>>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>>>>>> to define types only valid for the user space inside the copy of a >>>>>>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>>>>>> added later. >>>>>>>>> >>>>>>>>> This leads to the inevitable clash between user space and kernel types >>>>>>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>>>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>>>>>> >>>>>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>>>>>> older user space application, application tries to parse it as >>>>>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>>>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>>>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>>>>>> broken. >>>>>>>>> >>>>>>>>> Fixing that by bringing these user space attributes to the kernel >>>>>>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>>>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>>>>>> of the older user space applications at this point. >>>>>>>>> >>>>>>>>> These 2 types are explicitly rejected now since they should not be >>>>>>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>>>>>> it from the userspace. And it's also explicitly rejected now, because >>>>>>>>> it's for in-kernel use only. >>>>>>>>> >>>>>>>>> Comments with warnings were added to avoid the problem coming back. >>>>>>>>> >>>>>>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>>>>>> >>>>>>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>>>>>> >>>>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>>>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>>>>> --- >>>>>>>> >>>>>>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> I got to check traffic with the fix and I do get some traffic >>>>>>> but something is broken. I didn't investigate much but the quick >>>>>>> test shows me rules are not offloaded and dumping ovs rules gives >>>>>>> error like this >>>>>>> >>>>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>>>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>>>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>>>>> >>>>>> Such a dump is expected, because kernel parses fields that current >>>>>> userspace doesn't understand, and at the same time OVS by design is >>>>>> using kernel provided key/mask while installing datapath rules, IIRC. >>>>>> It should be possible to make these dumps a bit more friendly though. >>>>>> >>>>>> For the offloading not working, see my comment in the v2 patch email >>>>>> I sent (top email of this thread). In short, it's a problem in user >>>>>> space and it can not be fixed from the kernel side, unless we revert >>>>>> IPv6 extension header support and never add any new types, which is >>>>>> unreasonable. I didn't test any actual offloading, but I had a >>>>>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>>>>> the top email. >>>>> >>>>> Hi Ilya, >>>>> >>>>> I can confirm that with latest OvS master IPv6 rules offload still fails >>>>> without your pastebin code applied. >>>>> >>>>>> >>>>>> Since we're here: >>>>>> >>>>>> Toms, do you plan to submit user space patches for this feature? >>>>> >>>>> I see there is a patch from you that is supposed to fix compatibility >>>>> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >>>>> uAPI definition with the kernel."), but it doesn't fix offload for me >>>>> without pastebin patch. >>>> >>>> Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. >>>> Issue with offload is an OVS bug that should be fixed separately. >>>> The fix will also need to be backported to OVS stable branches. >>>> >>>>> Do you plan to merge that code into OvS or you >>>>> require some help from our side? >>>> >>>> I could do that, but I don't really have enough time. So, if you >>>> can work on that fix, it would be great. Note that comments inside >>>> the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly >>>> copied from the userspace datapath and are incorrect for the general >>>> case, so has to be fixed alongside the logic of that function. >>> >>> Tom or Vlad, are you working on this? Asking, as the release of a kernel with >>> Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will >>> break OVS. >>> >>> //Eelco >> >> Hi Eelco, >> >> My simple fix for OvS was rejected and I don't have time to rework it at >> the moment. > > That’s a pity, Tom do you maybe have time as your patch left OVS in this error state? Looks like everybody is busy, and as the patched kernel is now available, let me try to fix this on the OVS side. //Eelco
On 23 May 2022, at 14:54, Eelco Chaudron wrote: > On 17 May 2022, at 13:10, Eelco Chaudron wrote: > >> On 12 May 2022, at 12:08, Vlad Buslov wrote: >> >>> On Thu 12 May 2022 at 12:19, Eelco Chaudron <echaudro@redhat.com> wrote: >>>> On 7 Apr 2022, at 12:22, Ilya Maximets wrote: >>>> >>>>> On 4/7/22 10:02, Vlad Buslov wrote: >>>>>> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@ovn.org> wrote: >>>>>>> On 3/14/22 19:33, Roi Dayan wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>>>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>>>>>> >>>>>>>>>> Few years ago OVS user space made a strange choice in the commit [1] >>>>>>>>>> to define types only valid for the user space inside the copy of a >>>>>>>>>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >>>>>>>>>> added later. >>>>>>>>>> >>>>>>>>>> This leads to the inevitable clash between user space and kernel types >>>>>>>>>> when the kernel uAPI is extended. The issue was unveiled with the >>>>>>>>>> addition of a new type for IPv6 extension header in kernel uAPI. >>>>>>>>>> >>>>>>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >>>>>>>>>> older user space application, application tries to parse it as >>>>>>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >>>>>>>>>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >>>>>>>>>> every IPv6 packet that goes to the user space, IPv6 support is fully >>>>>>>>>> broken. >>>>>>>>>> >>>>>>>>>> Fixing that by bringing these user space attributes to the kernel >>>>>>>>>> uAPI to avoid the clash. Strictly speaking this is not the problem >>>>>>>>>> of the kernel uAPI, but changing it is the only way to avoid breakage >>>>>>>>>> of the older user space applications at this point. >>>>>>>>>> >>>>>>>>>> These 2 types are explicitly rejected now since they should not be >>>>>>>>>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >>>>>>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to hide >>>>>>>>>> it from the userspace. And it's also explicitly rejected now, because >>>>>>>>>> it's for in-kernel use only. >>>>>>>>>> >>>>>>>>>> Comments with warnings were added to avoid the problem coming back. >>>>>>>>>> >>>>>>>>>> (1 << type) converted to (1ULL << type) to avoid integer overflow on >>>>>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >>>>>>>>>> >>>>>>>>>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >>>>>>>>>> >>>>>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") >>>>>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com >>>>>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >>>>>>>>>> Reported-by: Roi Dayan <roid@nvidia.com> >>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>>>>>> --- >>>>>>>>> >>>>>>>>> Acked-by: Aaron Conole <aconole@redhat.com> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> I got to check traffic with the fix and I do get some traffic >>>>>>>> but something is broken. I didn't investigate much but the quick >>>>>>>> test shows me rules are not offloaded and dumping ovs rules gives >>>>>>>> error like this >>>>>>>> >>>>>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>>>>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>>>>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>>>>>> >>>>>>> Such a dump is expected, because kernel parses fields that current >>>>>>> userspace doesn't understand, and at the same time OVS by design is >>>>>>> using kernel provided key/mask while installing datapath rules, IIRC. >>>>>>> It should be possible to make these dumps a bit more friendly though. >>>>>>> >>>>>>> For the offloading not working, see my comment in the v2 patch email >>>>>>> I sent (top email of this thread). In short, it's a problem in user >>>>>>> space and it can not be fixed from the kernel side, unless we revert >>>>>>> IPv6 extension header support and never add any new types, which is >>>>>>> unreasonable. I didn't test any actual offloading, but I had a >>>>>>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>>>>>> the top email. >>>>>> >>>>>> Hi Ilya, >>>>>> >>>>>> I can confirm that with latest OvS master IPv6 rules offload still fails >>>>>> without your pastebin code applied. >>>>>> >>>>>>> >>>>>>> Since we're here: >>>>>>> >>>>>>> Toms, do you plan to submit user space patches for this feature? >>>>>> >>>>>> I see there is a patch from you that is supposed to fix compatibility >>>>>> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >>>>>> uAPI definition with the kernel."), but it doesn't fix offload for me >>>>>> without pastebin patch. >>>>> >>>>> Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. >>>>> Issue with offload is an OVS bug that should be fixed separately. >>>>> The fix will also need to be backported to OVS stable branches. >>>>> >>>>>> Do you plan to merge that code into OvS or you >>>>>> require some help from our side? >>>>> >>>>> I could do that, but I don't really have enough time. So, if you >>>>> can work on that fix, it would be great. Note that comments inside >>>>> the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly >>>>> copied from the userspace datapath and are incorrect for the general >>>>> case, so has to be fixed alongside the logic of that function. >>>> >>>> Tom or Vlad, are you working on this? Asking, as the release of a kernel with >>>> Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will >>>> break OVS. >>>> >>>> //Eelco >>> >>> Hi Eelco, >>> >>> My simple fix for OvS was rejected and I don't have time to rework it at >>> the moment. >> >> That’s a pity, Tom do you maybe have time as your patch left OVS in this error state? > > Looks like everybody is busy, and as the patched kernel is now available, let me try to fix this on the OVS side. I posted a patch on the OVS mailing list, please review/verify. https://patchwork.ozlabs.org/project/openvswitch/patch/165400753240.2054605.12815593709246812392.stgit@ebuild/ //Eelco
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index 9d1710f20505..ce3e1738d427 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -351,11 +351,21 @@ enum ovs_key_attr { OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */ OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */ OVS_KEY_ATTR_NSH, /* Nested set of ovs_nsh_key_* */ - OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */ -#ifdef __KERNEL__ - OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */ -#endif + /* User space decided to squat on types 29 and 30. They are defined + * below, but should not be sent to the kernel. + * + * WARNING: No new types should be added unless they are defined + * for both kernel and user space (no 'ifdef's). It's hard + * to keep compatibility otherwise. + */ + OVS_KEY_ATTR_PACKET_TYPE, /* be32 packet type */ + OVS_KEY_ATTR_ND_EXTENSIONS, /* IPv6 Neighbor Discovery extensions */ + + OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info. + * For in-kernel use only. + */ + OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */ __OVS_KEY_ATTR_MAX }; diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index 8b4124820f7d..5176f6ccac8e 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void) /* Whenever adding new OVS_KEY_ FIELDS, we should consider * updating this function. */ - BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30); + BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32); return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */ + nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */ @@ -482,7 +482,14 @@ static int __parse_flow_nlattrs(const struct nlattr *attr, return -EINVAL; } - if (attrs & (1 << type)) { + if (type == OVS_KEY_ATTR_PACKET_TYPE || + type == OVS_KEY_ATTR_ND_EXTENSIONS || + type == OVS_KEY_ATTR_TUNNEL_INFO) { + OVS_NLERR(log, "Key type %d is not supported", type); + return -EINVAL; + } + + if (attrs & (1ULL << type)) { OVS_NLERR(log, "Duplicate key (type %d).", type); return -EINVAL; } @@ -495,7 +502,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr, } if (!nz || !is_all_zero(nla_data(nla), nla_len(nla))) { - attrs |= 1 << type; + attrs |= 1ULL << type; a[type] = nla; } }
Few years ago OVS user space made a strange choice in the commit [1] to define types only valid for the user space inside the copy of a kernel uAPI header. '#ifndef __KERNEL__' and another attribute was added later. This leads to the inevitable clash between user space and kernel types when the kernel uAPI is extended. The issue was unveiled with the addition of a new type for IPv6 extension header in kernel uAPI. When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the older user space application, application tries to parse it as OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with every IPv6 packet that goes to the user space, IPv6 support is fully broken. Fixing that by bringing these user space attributes to the kernel uAPI to avoid the clash. Strictly speaking this is not the problem of the kernel uAPI, but changing it is the only way to avoid breakage of the older user space applications at this point. These 2 types are explicitly rejected now since they should not be passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved out from the '#ifdef __KERNEL__' as there is no good reason to hide it from the userspace. And it's also explicitly rejected now, because it's for in-kernel use only. Comments with warnings were added to avoid the problem coming back. (1 << type) converted to (1ULL << type) to avoid integer overflow on OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support") Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8a5f@nvidia.com Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c Reported-by: Roi Dayan <roid@nvidia.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Version 2: - (1 << type) --> (1ULL << type) to fix the integer overflow. - Tested with OVS 2.13 LTS and latest OVS 2.17 releases: * 'make check-kernel' and 'check-system-userspace' testsuites succeeded. ('make check-kernel' fails with the current net-next without the patch) * 'make check-offloads' fails HW offloading tests, but this is not a kernel's fault. OVS offloading fails to handle perfectly valid cases where kernel and user space are parsing different number of packet fields (ODP_FIT_TOO_LITTLE and ODP_FIT_TOO_MUCH cases). The problem is purely in user space and can not be fixed by kernel changes. FWIW, quick'n'dirty version of a fix for the OVS user space may look like this: https://pastebin.com/qR0ZjvQ7 include/uapi/linux/openvswitch.h | 18 ++++++++++++++---- net/openvswitch/flow_netlink.c | 13 ++++++++++--- 2 files changed, 24 insertions(+), 7 deletions(-)