| Message ID | 20210915150803.512312-1-mark.d.gray@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] dpif-netlink: Fix feature negotiation for older kernels | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 9/15/21 5:08 PM, Mark Gray wrote: > Older kernels do not reject unsupported features. This can lead > to a situation in which 'ovs-vswitchd' believes that a feature is > supported when, in fact, it is not. > > This patch probes for this by attempting to set a known unsupported > feature. > > Reported-by: Dumitru Ceara <dceara@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- This works for me, for this version: Tested-by: Dumitru Ceara <dceara@redhat.com> I do have a small comment below, thanks! > > Notes: > v2: Fix case raised by Dumitru in which kernel is already configured with > unsupported features. > > lib/dpif-netlink.c | 72 ++++++++++++++++++++++++++++++++-------------- > 1 file changed, 50 insertions(+), 22 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 34fc04237333..5f4b60c5a6d6 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -84,6 +84,8 @@ enum { MAX_PORTS = USHRT_MAX }; > #define EPOLLEXCLUSIVE (1u << 28) > #endif > > +#define OVS_DP_F_UNSUPPORTED (1 << 31); > + > /* This PID is not used by the kernel datapath when using dispatch per CPU, > * but it is required to be set (not zero). */ > #define DPIF_NETLINK_PER_CPU_PID UINT32_MAX > @@ -382,36 +384,62 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, > dp_request.cmd = OVS_DP_CMD_SET; > } > > - /* The Open vSwitch kernel module has two modes for dispatching upcalls: > - * per-vport and per-cpu. > - * > - * When dispatching upcalls per-vport, the kernel will > - * send the upcall via a Netlink socket that has been selected based on the > - * vport that received the packet that is causing the upcall. > - * > - * When dispatching upcall per-cpu, the kernel will send the upcall via > - * a Netlink socket that has been selected based on the cpu that received > - * the packet that is causing the upcall. > - * > - * First we test to see if the kernel module supports per-cpu dispatching > - * (the preferred method). If it does not support per-cpu dispatching, we > - * fall back to the per-vport dispatch mode. > + /* Some older kernels will not reject unknown features. This will cause > + * 'ovs-vswitchd' to incorrectly assume a feature is supported. In order to > + * test for that, we attempt to set a feature that we know is not supported > + * by any kernel. If this feature is not rejected, we can assume we are > + * running on one of these older kernels. > */ > dp_request.user_features |= OVS_DP_F_UNALIGNED; > - dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; > - dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; > + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > + dp_request.user_features |= OVS_DP_F_UNSUPPORTED; Nit: Sorry, I missed this on v1, but I guess we could just remove the lines that set OVS_DP_F_UNALIGNED and OVS_DP_F_VPORT_PIDS here, and just request OVS_DP_F_UNSUPPORTED. We set the correct features anyway, in all cases, below. As far as I can tell this doesn't affect functionality; I tested with the two lines removed and it looks OK to me. If you send a v3 removing these two lines please feel free to keep the "tested-by". > error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > if (error) { > - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; > + /* The Open vSwitch kernel module has two modes for dispatching > + * upcalls: per-vport and per-cpu. > + * > + * When dispatching upcalls per-vport, the kernel will > + * send the upcall via a Netlink socket that has been selected based on > + * the vport that received the packet that is causing the upcall. > + * > + * When dispatching upcall per-cpu, the kernel will send the upcall via > + * a Netlink socket that has been selected based on the cpu that > + * received the packet that is causing the upcall. > + * > + * First we test to see if the kernel module supports per-cpu > + * dispatching (the preferred method). If it does not support per-cpu > + * dispatching, we fall back to the per-vport dispatch mode. > + */ > + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; > + dp_request.user_features |= OVS_DP_F_UNALIGNED; > + dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; > + dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; > + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > + if (error) { > + dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; > + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > + } > + if (error) { > + return error; > + } > + > + error = open_dpif(&dp, dpifp); > + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); > + } else { > + VLOG_INFO("Kernel does not correctly support feature negotiation. " > + "Using standard features."); > + dp_request.cmd = OVS_DP_CMD_SET; > + dp_request.user_features = 0; > + dp_request.user_features |= OVS_DP_F_UNALIGNED; > dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > - } > - if (error) { > - return error; > + if (error) { > + return error; > + } > + error = open_dpif(&dp, dpifp); > } > > - error = open_dpif(&dp, dpifp); > - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); > ofpbuf_delete(buf); > > if (create) { >
On 16/09/2021 09:40, Dumitru Ceara wrote: > On 9/15/21 5:08 PM, Mark Gray wrote: >> Older kernels do not reject unsupported features. This can lead >> to a situation in which 'ovs-vswitchd' believes that a feature is >> supported when, in fact, it is not. >> >> This patch probes for this by attempting to set a known unsupported >> feature. >> >> Reported-by: Dumitru Ceara <dceara@redhat.com> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> --- > > This works for me, for this version: > > Tested-by: Dumitru Ceara <dceara@redhat.com> > > I do have a small comment below, thanks! > >> >> Notes: >> v2: Fix case raised by Dumitru in which kernel is already configured with >> unsupported features. >> >> lib/dpif-netlink.c | 72 ++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 50 insertions(+), 22 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index 34fc04237333..5f4b60c5a6d6 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -84,6 +84,8 @@ enum { MAX_PORTS = USHRT_MAX }; >> #define EPOLLEXCLUSIVE (1u << 28) >> #endif >> >> +#define OVS_DP_F_UNSUPPORTED (1 << 31); >> + >> /* This PID is not used by the kernel datapath when using dispatch per CPU, >> * but it is required to be set (not zero). */ >> #define DPIF_NETLINK_PER_CPU_PID UINT32_MAX >> @@ -382,36 +384,62 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, >> dp_request.cmd = OVS_DP_CMD_SET; >> } >> >> - /* The Open vSwitch kernel module has two modes for dispatching upcalls: >> - * per-vport and per-cpu. >> - * >> - * When dispatching upcalls per-vport, the kernel will >> - * send the upcall via a Netlink socket that has been selected based on the >> - * vport that received the packet that is causing the upcall. >> - * >> - * When dispatching upcall per-cpu, the kernel will send the upcall via >> - * a Netlink socket that has been selected based on the cpu that received >> - * the packet that is causing the upcall. >> - * >> - * First we test to see if the kernel module supports per-cpu dispatching >> - * (the preferred method). If it does not support per-cpu dispatching, we >> - * fall back to the per-vport dispatch mode. >> + /* Some older kernels will not reject unknown features. This will cause >> + * 'ovs-vswitchd' to incorrectly assume a feature is supported. In order to >> + * test for that, we attempt to set a feature that we know is not supported >> + * by any kernel. If this feature is not rejected, we can assume we are >> + * running on one of these older kernels. >> */ >> dp_request.user_features |= OVS_DP_F_UNALIGNED; >> - dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; >> - dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >> + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >> + dp_request.user_features |= OVS_DP_F_UNSUPPORTED; > > Nit: Sorry, I missed this on v1, but I guess we could just remove the > lines that set OVS_DP_F_UNALIGNED and OVS_DP_F_VPORT_PIDS here, and just > request OVS_DP_F_UNSUPPORTED. We set the correct features anyway, in > all cases, below. I don't want to do that because it would change the behaviour of the "create" case. In the previous code, when we are creating the datapath, we set UNALIGNED, VPORT_PIDs using the NEW command. If I remove that, I will call NEW without the flags and then SET them flags after. Not sure if it will have any adverse consequences on some version of the kernel so it may not be worth the risk. What do you think? > > As far as I can tell this doesn't affect functionality; I tested with > the two lines removed and it looks OK to me. If you send a v3 removing > these two lines please feel free to keep the "tested-by". > >> error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); >> if (error) { >> - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >> + /* The Open vSwitch kernel module has two modes for dispatching >> + * upcalls: per-vport and per-cpu. >> + * >> + * When dispatching upcalls per-vport, the kernel will >> + * send the upcall via a Netlink socket that has been selected based on >> + * the vport that received the packet that is causing the upcall. >> + * >> + * When dispatching upcall per-cpu, the kernel will send the upcall via >> + * a Netlink socket that has been selected based on the cpu that >> + * received the packet that is causing the upcall. >> + * >> + * First we test to see if the kernel module supports per-cpu >> + * dispatching (the preferred method). If it does not support per-cpu >> + * dispatching, we fall back to the per-vport dispatch mode. >> + */ >> + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; >> + dp_request.user_features |= OVS_DP_F_UNALIGNED; >> + dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; >> + dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >> + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); >> + if (error) { >> + dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >> + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >> + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); >> + } >> + if (error) { >> + return error; >> + } >> + >> + error = open_dpif(&dp, dpifp); >> + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); >> + } else { >> + VLOG_INFO("Kernel does not correctly support feature negotiation. " >> + "Using standard features."); >> + dp_request.cmd = OVS_DP_CMD_SET; >> + dp_request.user_features = 0; >> + dp_request.user_features |= OVS_DP_F_UNALIGNED; >> dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >> error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); >> - } >> - if (error) { >> - return error; >> + if (error) { >> + return error; >> + } >> + error = open_dpif(&dp, dpifp); >> } >> >> - error = open_dpif(&dp, dpifp); >> - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); >> ofpbuf_delete(buf); >> >> if (create) { >> >
On 9/16/21 12:04 PM, Mark Gray wrote: > On 16/09/2021 09:40, Dumitru Ceara wrote: >> On 9/15/21 5:08 PM, Mark Gray wrote: >>> Older kernels do not reject unsupported features. This can lead >>> to a situation in which 'ovs-vswitchd' believes that a feature is >>> supported when, in fact, it is not. >>> >>> This patch probes for this by attempting to set a known unsupported >>> feature. >>> >>> Reported-by: Dumitru Ceara <dceara@redhat.com> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 >>> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >>> --- >> >> This works for me, for this version: >> >> Tested-by: Dumitru Ceara <dceara@redhat.com> >> >> I do have a small comment below, thanks! >> >>> >>> Notes: >>> v2: Fix case raised by Dumitru in which kernel is already configured with >>> unsupported features. >>> >>> lib/dpif-netlink.c | 72 ++++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 50 insertions(+), 22 deletions(-) >>> >>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>> index 34fc04237333..5f4b60c5a6d6 100644 >>> --- a/lib/dpif-netlink.c >>> +++ b/lib/dpif-netlink.c >>> @@ -84,6 +84,8 @@ enum { MAX_PORTS = USHRT_MAX }; >>> #define EPOLLEXCLUSIVE (1u << 28) >>> #endif >>> >>> +#define OVS_DP_F_UNSUPPORTED (1 << 31); >>> + >>> /* This PID is not used by the kernel datapath when using dispatch per CPU, >>> * but it is required to be set (not zero). */ >>> #define DPIF_NETLINK_PER_CPU_PID UINT32_MAX >>> @@ -382,36 +384,62 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, >>> dp_request.cmd = OVS_DP_CMD_SET; >>> } >>> >>> - /* The Open vSwitch kernel module has two modes for dispatching upcalls: >>> - * per-vport and per-cpu. >>> - * >>> - * When dispatching upcalls per-vport, the kernel will >>> - * send the upcall via a Netlink socket that has been selected based on the >>> - * vport that received the packet that is causing the upcall. >>> - * >>> - * When dispatching upcall per-cpu, the kernel will send the upcall via >>> - * a Netlink socket that has been selected based on the cpu that received >>> - * the packet that is causing the upcall. >>> - * >>> - * First we test to see if the kernel module supports per-cpu dispatching >>> - * (the preferred method). If it does not support per-cpu dispatching, we >>> - * fall back to the per-vport dispatch mode. >>> + /* Some older kernels will not reject unknown features. This will cause >>> + * 'ovs-vswitchd' to incorrectly assume a feature is supported. In order to >>> + * test for that, we attempt to set a feature that we know is not supported >>> + * by any kernel. If this feature is not rejected, we can assume we are >>> + * running on one of these older kernels. >>> */ >>> dp_request.user_features |= OVS_DP_F_UNALIGNED; >>> - dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; >>> - dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >>> + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >>> + dp_request.user_features |= OVS_DP_F_UNSUPPORTED; >> >> Nit: Sorry, I missed this on v1, but I guess we could just remove the >> lines that set OVS_DP_F_UNALIGNED and OVS_DP_F_VPORT_PIDS here, and just >> request OVS_DP_F_UNSUPPORTED. We set the correct features anyway, in >> all cases, below. > > I don't want to do that because it would change the behaviour of the > "create" case. In the previous code, when we are creating the datapath, > we set UNALIGNED, VPORT_PIDs using the NEW command. If I remove that, I > will call NEW without the flags and then SET them flags after. Not sure > if it will have any adverse consequences on some version of the kernel > so it may not be worth the risk. What do you think? > Ok, I see now, thanks for the follow up! Let's leave it as is then. Regards, Dumitru
On 9/16/21 10:40, Dumitru Ceara wrote: > On 9/15/21 5:08 PM, Mark Gray wrote: >> Older kernels do not reject unsupported features. This can lead >> to a situation in which 'ovs-vswitchd' believes that a feature is >> supported when, in fact, it is not. >> >> This patch probes for this by attempting to set a known unsupported >> feature. >> >> Reported-by: Dumitru Ceara <dceara@redhat.com> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> --- > > This works for me, for this version: > > Tested-by: Dumitru Ceara <dceara@redhat.com> Thanks! Applied and backported to 2.16. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 34fc04237333..5f4b60c5a6d6 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -84,6 +84,8 @@ enum { MAX_PORTS = USHRT_MAX }; #define EPOLLEXCLUSIVE (1u << 28) #endif +#define OVS_DP_F_UNSUPPORTED (1 << 31); + /* This PID is not used by the kernel datapath when using dispatch per CPU, * but it is required to be set (not zero). */ #define DPIF_NETLINK_PER_CPU_PID UINT32_MAX @@ -382,36 +384,62 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, dp_request.cmd = OVS_DP_CMD_SET; } - /* The Open vSwitch kernel module has two modes for dispatching upcalls: - * per-vport and per-cpu. - * - * When dispatching upcalls per-vport, the kernel will - * send the upcall via a Netlink socket that has been selected based on the - * vport that received the packet that is causing the upcall. - * - * When dispatching upcall per-cpu, the kernel will send the upcall via - * a Netlink socket that has been selected based on the cpu that received - * the packet that is causing the upcall. - * - * First we test to see if the kernel module supports per-cpu dispatching - * (the preferred method). If it does not support per-cpu dispatching, we - * fall back to the per-vport dispatch mode. + /* Some older kernels will not reject unknown features. This will cause + * 'ovs-vswitchd' to incorrectly assume a feature is supported. In order to + * test for that, we attempt to set a feature that we know is not supported + * by any kernel. If this feature is not rejected, we can assume we are + * running on one of these older kernels. */ dp_request.user_features |= OVS_DP_F_UNALIGNED; - dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; - dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; + dp_request.user_features |= OVS_DP_F_UNSUPPORTED; error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); if (error) { - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; + /* The Open vSwitch kernel module has two modes for dispatching + * upcalls: per-vport and per-cpu. + * + * When dispatching upcalls per-vport, the kernel will + * send the upcall via a Netlink socket that has been selected based on + * the vport that received the packet that is causing the upcall. + * + * When dispatching upcall per-cpu, the kernel will send the upcall via + * a Netlink socket that has been selected based on the cpu that + * received the packet that is causing the upcall. + * + * First we test to see if the kernel module supports per-cpu + * dispatching (the preferred method). If it does not support per-cpu + * dispatching, we fall back to the per-vport dispatch mode. + */ + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; + dp_request.user_features |= OVS_DP_F_UNALIGNED; + dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS; + dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU; + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); + if (error) { + dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; + error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); + } + if (error) { + return error; + } + + error = open_dpif(&dp, dpifp); + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); + } else { + VLOG_INFO("Kernel does not correctly support feature negotiation. " + "Using standard features."); + dp_request.cmd = OVS_DP_CMD_SET; + dp_request.user_features = 0; + dp_request.user_features |= OVS_DP_F_UNALIGNED; dp_request.user_features |= OVS_DP_F_VPORT_PIDS; error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); - } - if (error) { - return error; + if (error) { + return error; + } + error = open_dpif(&dp, dpifp); } - error = open_dpif(&dp, dpifp); - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); ofpbuf_delete(buf); if (create) {
Older kernels do not reject unsupported features. This can lead to a situation in which 'ovs-vswitchd' believes that a feature is supported when, in fact, it is not. This patch probes for this by attempting to set a known unsupported feature. Reported-by: Dumitru Ceara <dceara@redhat.com> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- Notes: v2: Fix case raised by Dumitru in which kernel is already configured with unsupported features. lib/dpif-netlink.c | 72 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 22 deletions(-)