[ovs-dev] [PATCH v2] dpif-netlink: Fix feature negotiation for older kernels

[Dumitru Ceara]

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 at redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083
> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> ---

This works for me, for this version:

Tested-by: Dumitru Ceara <dceara at 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) {
>