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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3ad7bdd2-80d2-4d73-b86f-4c0aeeee5bf1@intel.com>
Date: Wed, 18 Dec 2024 16:30:49 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Praveen Kaligineedi <pkaligineedi@...gle.com>
CC: <netdev@...r.kernel.org>, <jeroendb@...gle.com>, <shailend@...gle.com>,
	<willemb@...gle.com>, <andrew+netdev@...n.ch>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<ast@...nel.org>, <daniel@...earbox.net>, <hawk@...nel.org>,
	<john.fastabend@...il.com>, <horms@...nel.org>, <hramamurthy@...gle.com>,
	<joshwash@...gle.com>, <ziweixiao@...gle.com>,
	<linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
	<stable@...r.kernel.org>
Subject: Re: [PATCH net 2/5] gve: guard XDP xmit NDO on existence of xdp
 queues

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

> From: Joshua Washington <joshwash@...gle.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@...r.kernel.org
> Signed-off-by: Joshua Washington <joshwash@...gle.com>
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@...gle.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@...gle.com>
> Reviewed-by: Shailend Chand <shailend@...gle.com>
> Reviewed-by: Willem de Bruijn <willemb@...gle.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