Re: [PATCH net 2/5] gve: guard XDP xmit NDO on existence of xdp queues

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Praveen Kaligineedi <pkaligineedi@google.com>
Date: Wed, 18 Dec 2024 05:34:12 -0800

> From: Joshua Washington <joshwash@google.com>
> 
> In GVE, dedicated XDP queues only exist when an XDP program is installed
> and the interface is up. As such, the NDO XDP XMIT callback should
> return early if either of these conditions are false.
> 
> In the case of no loaded XDP program, priv->num_xdp_queues=0 which can
> cause a divide-by-zero error, and in the case of interface down,
> num_xdp_queues remains untouched to persist XDP queue count for the next
> interface up, but the TX pointer itself would be NULL.
> 
> The XDP xmit callback also needs to synchronize with a device
> transitioning from open to close. This synchronization will happen via
> the GVE_PRIV_FLAGS_NAPI_ENABLED bit along with a synchronize_net() call,
> which waits for any RCU critical sections at call-time to complete.
> 
> Fixes: 39a7f4aa3e4a ("gve: Add XDP REDIRECT support for GQI-QPL format")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Shailend Chand <shailend@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/ethernet/google/gve/gve_main.c | 3 +++
>  drivers/net/ethernet/google/gve/gve_tx.c   | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index e171ca248f9a..5d7b0cc59959 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1899,6 +1899,9 @@ static void gve_turndown(struct gve_priv *priv)
>  
>  	gve_clear_napi_enabled(priv);
>  	gve_clear_report_stats(priv);
> +
> +	/* Make sure that all traffic is finished processing. */
> +	synchronize_net();

Wouldn't synchronize_rcu() be enough, have you checked?

>  }
>  
>  static void gve_turnup(struct gve_priv *priv)
> diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
> index 83ad278ec91f..852f8c7e39d2 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx.c
> @@ -837,9 +837,12 @@ int gve_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>  	struct gve_tx_ring *tx;
>  	int i, err = 0, qid;
>  
> -	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK) || !priv->xdp_prog)
>  		return -EINVAL;

The first condition (weird xmit flags) is certainly EINVAL.
Lack of installed XDP prog is *not*.

You need to use xdp_features_{set,clear}_redirect_target() when you
install/remove XDP prog to notify the kernel that ndo_start_xmit is now
available / not available anymore.

If you want to leave this check, I'd suggest changing it to

	if (unlikely(!priv->num_xdp_queues))
		return -ENXIO;

	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
		return -EINVAL;

>  
> +	if (!gve_get_napi_enabled(priv))
> +		return -ENETDOWN;
> +
>  	qid = gve_xdp_tx_queue_id(priv,
>  				  smp_processor_id() % priv->num_xdp_queues);

Thanks,
Olek