[PATCH net 0/5] gve: various XDP fixes

[PATCH net 0/5] gve: various XDP fixes

[Praveen Kaligineedi]

From: Joshua Washington <joshwash@google.com>

This patch series contains the following XDP fixes:
 - clean up XDP tx queue when stopping rings
 - use RCU synchronization to guard existence of XDP queues
 - perform XSK TX as part of RX NAPI to fix busy polling
 - fix XDP allocation issues when non-XDP configurations occur

Joshua Washington (5):
  gve: clean XDP queues in gve_tx_stop_ring_gqi
  gve: guard XDP xmit NDO on existence of xdp queues
  gve: guard XSK operations on the existence of queues
  gve: share napi for RX and XSK TX
  gve: fix XDP allocation path in edge cases

 drivers/net/ethernet/google/gve/gve.h      |  1 +
 drivers/net/ethernet/google/gve/gve_main.c | 42 ++++++++++++++------
 drivers/net/ethernet/google/gve/gve_tx.c   | 46 ++++++++++++++--------
 3 files changed, 60 insertions(+), 29 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH net 1/5] gve: clean XDP queues in gve_tx_stop_ring_gqi

[Praveen Kaligineedi]

From: Joshua Washington <joshwash@google.com>

When stopping XDP TX rings, the XDP clean function needs to be called to
clean out the entire queue, similar to what happens in the normal TX
queue case. Otherwise, the FIFO won't be cleared correctly, and
xsk_tx_completed won't be reported.

Fixes: 75eaae158b1b ("gve: Add XDP DROP and TX 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: Willem de Bruijn <willemb@google.com>
---
 drivers/net/ethernet/google/gve/gve_tx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
index e7fb7d6d283d..83ad278ec91f 100644
--- a/drivers/net/ethernet/google/gve/gve_tx.c
+++ b/drivers/net/ethernet/google/gve/gve_tx.c
@@ -206,7 +206,10 @@ void gve_tx_stop_ring_gqi(struct gve_priv *priv, int idx)
 		return;
 
 	gve_remove_napi(priv, ntfy_idx);
-	gve_clean_tx_done(priv, tx, priv->tx_desc_cnt, false);
+	if (tx->q_num < priv->tx_cfg.num_queues)
+		gve_clean_tx_done(priv, tx, priv->tx_desc_cnt, false);
+	else
+		gve_clean_xdp_done(priv, tx, priv->tx_desc_cnt);
 	netdev_tx_reset_queue(tx->netdev_txq);
 	gve_tx_remove_from_block(priv, idx);
 }
-- 
2.47.1.613.gc27f4b7a9f-goog

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

[Praveen Kaligineedi]

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();
 }
 
 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;
 
+	if (!gve_get_napi_enabled(priv))
+		return -ENETDOWN;
+
 	qid = gve_xdp_tx_queue_id(priv,
 				  smp_processor_id() % priv->num_xdp_queues);
 
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH net 3/5] gve: guard XSK operations on the existence of queues

[Praveen Kaligineedi]

From: Joshua Washington <joshwash@google.com>

This patch predicates the enabling and disabling of XSK pools on the
existence of queues. As it stands, if the interface is down, disabling
or enabling XSK pools would result in a crash, as the RX queue pointer
would be NULL. XSK pool registration will occur as part of the next
interface up.

Similarly, xsk_wakeup needs be guarded against queues disappearing
while the function is executing, so a check against the
GVE_PRIV_FLAGS_NAPI_ENABLED flag is added to synchronize with the
disabling of the bit and the synchronize_net() in gve_turndown.

Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy 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 | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 5d7b0cc59959..e4e8ff4f9f80 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1623,8 +1623,8 @@ static int gve_xsk_pool_enable(struct net_device *dev,
 	if (err)
 		return err;
 
-	/* If XDP prog is not installed, return */
-	if (!priv->xdp_prog)
+	/* If XDP prog is not installed or interface is down, return. */
+	if (!priv->xdp_prog || !netif_running(dev))
 		return 0;
 
 	rx = &priv->rx[qid];
@@ -1669,21 +1669,16 @@ static int gve_xsk_pool_disable(struct net_device *dev,
 	if (qid >= priv->rx_cfg.num_queues)
 		return -EINVAL;
 
-	/* If XDP prog is not installed, unmap DMA and return */
-	if (!priv->xdp_prog)
+	/* If XDP prog is not installed or interface is down, unmap DMA and
+	 * return.
+	 */
+	if (!priv->xdp_prog || !netif_running(dev))
 		goto done;
 
-	tx_qid = gve_xdp_tx_queue_id(priv, qid);
-	if (!netif_running(dev)) {
-		priv->rx[qid].xsk_pool = NULL;
-		xdp_rxq_info_unreg(&priv->rx[qid].xsk_rxq);
-		priv->tx[tx_qid].xsk_pool = NULL;
-		goto done;
-	}
-
 	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
 	napi_disable(napi_rx); /* make sure current rx poll is done */
 
+	tx_qid = gve_xdp_tx_queue_id(priv, qid);
 	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
 	napi_disable(napi_tx); /* make sure current tx poll is done */
 
@@ -1711,6 +1706,9 @@ static int gve_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
 	struct gve_priv *priv = netdev_priv(dev);
 	int tx_queue_id = gve_xdp_tx_queue_id(priv, queue_id);
 
+	if (!gve_get_napi_enabled(priv))
+		return -ENETDOWN;
+
 	if (queue_id >= priv->rx_cfg.num_queues || !priv->xdp_prog)
 		return -EINVAL;
 
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH net 4/5] gve: process XSK TX descriptors as part of RX NAPI

[Praveen Kaligineedi]

From: Joshua Washington <joshwash@google.com>

When busy polling is enabled, xsk_sendmsg for AF_XDP zero copy marks
the NAPI ID corresponding to the memory pool allocated for the socket.
In GVE, this NAPI ID will never correspond to a NAPI ID of one of the
dedicated XDP TX queues registered with the umem because XDP TX is not
set up to share a NAPI with a corresponding RX queue.

This patch moves XSK TX descriptor processing from the TX NAPI to the RX
NAPI, and the gve_xsk_wakeup callback is updated to use the RX NAPI
instead of the TX NAPI, accordingly. The branch on if the wakeup is for
TX is removed, as the NAPI poll should be invoked whether the wakeup is
for TX or for RX.

Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy support for GQI-QPL format")
Cc: stable@vger.kernel.org
Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/ethernet/google/gve/gve.h      |  1 +
 drivers/net/ethernet/google/gve/gve_main.c |  8 +++++
 drivers/net/ethernet/google/gve/gve_tx.c   | 36 +++++++++++++---------
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index dd92949bb214..8167cc5fb0df 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -1140,6 +1140,7 @@ int gve_xdp_xmit_one(struct gve_priv *priv, struct gve_tx_ring *tx,
 void gve_xdp_tx_flush(struct gve_priv *priv, u32 xdp_qid);
 bool gve_tx_poll(struct gve_notify_block *block, int budget);
 bool gve_xdp_poll(struct gve_notify_block *block, int budget);
+int gve_xsk_tx_poll(struct gve_notify_block *block, int budget);
 int gve_tx_alloc_rings_gqi(struct gve_priv *priv,
 			   struct gve_tx_alloc_rings_cfg *cfg);
 void gve_tx_free_rings_gqi(struct gve_priv *priv,
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index e4e8ff4f9f80..5cab7b88610f 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -333,6 +333,14 @@ int gve_napi_poll(struct napi_struct *napi, int budget)
 
 	if (block->rx) {
 		work_done = gve_rx_poll(block, budget);
+
+		/* Poll XSK TX as part of RX NAPI. Setup re-poll based on max of
+		 * TX and RX work done.
+		 */
+		if (priv->xdp_prog)
+			work_done = max_t(int, work_done,
+					  gve_xsk_tx_poll(block, budget));
+
 		reschedule |= work_done == budget;
 	}
 
diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
index 852f8c7e39d2..4350ebd9c2bd 100644
--- a/drivers/net/ethernet/google/gve/gve_tx.c
+++ b/drivers/net/ethernet/google/gve/gve_tx.c
@@ -981,33 +981,41 @@ static int gve_xsk_tx(struct gve_priv *priv, struct gve_tx_ring *tx,
 	return sent;
 }
 
+int gve_xsk_tx_poll(struct gve_notify_block *rx_block, int budget)
+{
+	struct gve_rx_ring *rx = rx_block->rx;
+	struct gve_priv *priv = rx->gve;
+	struct gve_tx_ring *tx;
+	int sent = 0;
+
+	tx = &priv->tx[gve_xdp_tx_queue_id(priv, rx->q_num)];
+	if (tx->xsk_pool) {
+		sent = gve_xsk_tx(priv, tx, budget);
+
+		u64_stats_update_begin(&tx->statss);
+		tx->xdp_xsk_sent += sent;
+		u64_stats_update_end(&tx->statss);
+		if (xsk_uses_need_wakeup(tx->xsk_pool))
+			xsk_set_tx_need_wakeup(tx->xsk_pool);
+	}
+
+	return sent;
+}
+
 bool gve_xdp_poll(struct gve_notify_block *block, int budget)
 {
 	struct gve_priv *priv = block->priv;
 	struct gve_tx_ring *tx = block->tx;
 	u32 nic_done;
-	bool repoll;
 	u32 to_do;
 
 	/* Find out how much work there is to be done */
 	nic_done = gve_tx_load_event_counter(priv, tx);
 	to_do = min_t(u32, (nic_done - tx->done), budget);
 	gve_clean_xdp_done(priv, tx, to_do);
-	repoll = nic_done != tx->done;
-
-	if (tx->xsk_pool) {
-		int sent = gve_xsk_tx(priv, tx, budget);
-
-		u64_stats_update_begin(&tx->statss);
-		tx->xdp_xsk_sent += sent;
-		u64_stats_update_end(&tx->statss);
-		repoll |= (sent == budget);
-		if (xsk_uses_need_wakeup(tx->xsk_pool))
-			xsk_set_tx_need_wakeup(tx->xsk_pool);
-	}
 
 	/* If we still have work we want to repoll */
-	return repoll;
+	return nic_done != tx->done;
 }
 
 bool gve_tx_poll(struct gve_notify_block *block, int budget)
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH net 5/5] gve: fix XDP allocation path in edge cases

[Praveen Kaligineedi]

From: Joshua Washington <joshwash@google.com>

This patch fixes a number of consistency issues in the queue allocation
path related to XDP.

As it stands, the number of allocated XDP queues changes in three
different scenarios.
1) Adding an XDP program while the interface is up via
   gve_add_xdp_queues
2) Removing an XDP program while the interface is up via
   gve_remove_xdp_queues
3) After queues have been allocated and the old queue memory has been
   removed in gve_queues_start.

However, the requirement for the interface to be up for
gve_(add|remove)_xdp_queues to be called, in conjunction with the fact
that the number of queues stored in priv isn't updated until _after_ XDP
queues have been allocated in the normal queue allocation path means
that if an XDP program is added while the interface is down, XDP queues
won't be added until the _second_ if_up, not the first.

Given the expectation that the number of XDP queues is equal to the
number of RX queues, scenario (3) has another problematic implication.
When changing the number of queues while an XDP program is loaded, the
number of XDP queues must be updated as well, as there is logic in the
driver (gve_xdp_tx_queue_id()) which relies on every RX queue having a
corresponding XDP TX queue. However, the number of XDP queues stored in
priv would not be updated until _after_ a close/open leading to a
mismatch in the number of XDP queues reported vs the number of XDP
queues which actually exist after the queue count update completes.

This patch remedies these issues by doing the following:
1) The allocation config getter function is set up to retrieve the
   _expected_ number of XDP queues to allocate instead of relying
   on the value stored in `priv` which is only updated once the queues
   have been allocated.
2) When adjusting queues, XDP queues are adjusted to match the number of
   RX queues when XDP is enabled. This only works in the case when
   queues are live, so part (1) of the fix must still be available in
   the case that queues are adjusted when there is an XDP program and
   the interface is down.

Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues")
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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 5cab7b88610f..09fb7f16f73e 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -930,11 +930,13 @@ static void gve_init_sync_stats(struct gve_priv *priv)
 static void gve_tx_get_curr_alloc_cfg(struct gve_priv *priv,
 				      struct gve_tx_alloc_rings_cfg *cfg)
 {
+	int num_xdp_queues = priv->xdp_prog ? priv->rx_cfg.num_queues : 0;
+
 	cfg->qcfg = &priv->tx_cfg;
 	cfg->raw_addressing = !gve_is_qpl(priv);
 	cfg->ring_size = priv->tx_desc_cnt;
 	cfg->start_idx = 0;
-	cfg->num_rings = gve_num_tx_queues(priv);
+	cfg->num_rings = priv->tx_cfg.num_queues + num_xdp_queues;
 	cfg->tx = priv->tx;
 }
 
@@ -1843,6 +1845,7 @@ int gve_adjust_queues(struct gve_priv *priv,
 {
 	struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
 	struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
+	int num_xdp_queues;
 	int err;
 
 	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
@@ -1853,6 +1856,10 @@ int gve_adjust_queues(struct gve_priv *priv,
 	rx_alloc_cfg.qcfg = &new_rx_config;
 	tx_alloc_cfg.num_rings = new_tx_config.num_queues;
 
+	/* Add dedicated XDP TX queues if enabled. */
+	num_xdp_queues = priv->xdp_prog ? new_rx_config.num_queues : 0;
+	tx_alloc_cfg.num_rings += num_xdp_queues;
+
 	if (netif_running(priv->dev)) {
 		err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg);
 		return err;
-- 
2.47.1.613.gc27f4b7a9f-goog

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

[Alexander Lobakin]

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

Re: [PATCH net 3/5] gve: guard XSK operations on the existence of queues

[Larysa Zaremba]

On Wed, Dec 18, 2024 at 05:34:13AM -0800, Praveen Kaligineedi wrote:
> From: Joshua Washington <joshwash@google.com>
> 
> This patch predicates the enabling and disabling of XSK pools on the
> existence of queues. As it stands, if the interface is down, disabling
> or enabling XSK pools would result in a crash, as the RX queue pointer
> would be NULL. XSK pool registration will occur as part of the next
> interface up.
> 
> Similarly, xsk_wakeup needs be guarded against queues disappearing
> while the function is executing, so a check against the
> GVE_PRIV_FLAGS_NAPI_ENABLED flag is added to synchronize with the
> disabling of the bit and the synchronize_net() in gve_turndown.
> 
> Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy 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>

I think the sender's SoB still should be last, but otherwise looks good.

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

> ---
>  drivers/net/ethernet/google/gve/gve_main.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 5d7b0cc59959..e4e8ff4f9f80 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1623,8 +1623,8 @@ static int gve_xsk_pool_enable(struct net_device *dev,
>  	if (err)
>  		return err;
>  
> -	/* If XDP prog is not installed, return */
> -	if (!priv->xdp_prog)
> +	/* If XDP prog is not installed or interface is down, return. */
> +	if (!priv->xdp_prog || !netif_running(dev))
>  		return 0;
>  
>  	rx = &priv->rx[qid];
> @@ -1669,21 +1669,16 @@ static int gve_xsk_pool_disable(struct net_device *dev,
>  	if (qid >= priv->rx_cfg.num_queues)
>  		return -EINVAL;
>  
> -	/* If XDP prog is not installed, unmap DMA and return */
> -	if (!priv->xdp_prog)
> +	/* If XDP prog is not installed or interface is down, unmap DMA and
> +	 * return.
> +	 */
> +	if (!priv->xdp_prog || !netif_running(dev))
>  		goto done;
>  
> -	tx_qid = gve_xdp_tx_queue_id(priv, qid);
> -	if (!netif_running(dev)) {
> -		priv->rx[qid].xsk_pool = NULL;
> -		xdp_rxq_info_unreg(&priv->rx[qid].xsk_rxq);
> -		priv->tx[tx_qid].xsk_pool = NULL;
> -		goto done;
> -	}
> -
>  	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
>  	napi_disable(napi_rx); /* make sure current rx poll is done */
>  
> +	tx_qid = gve_xdp_tx_queue_id(priv, qid);
>  	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
>  	napi_disable(napi_tx); /* make sure current tx poll is done */
>  
> @@ -1711,6 +1706,9 @@ static int gve_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
>  	struct gve_priv *priv = netdev_priv(dev);
>  	int tx_queue_id = gve_xdp_tx_queue_id(priv, queue_id);
>  
> +	if (!gve_get_napi_enabled(priv))
> +		return -ENETDOWN;
> +
>  	if (queue_id >= priv->rx_cfg.num_queues || !priv->xdp_prog)
>  		return -EINVAL;
>  
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 
> 

Re: [PATCH net 0/5] gve: various XDP fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 18 Dec 2024 05:34:10 -0800 you wrote:
> From: Joshua Washington <joshwash@google.com>
> 
> This patch series contains the following XDP fixes:
>  - clean up XDP tx queue when stopping rings
>  - use RCU synchronization to guard existence of XDP queues
>  - perform XSK TX as part of RX NAPI to fix busy polling
>  - fix XDP allocation issues when non-XDP configurations occur
> 
> [...]

Here is the summary with links:
  - [net,1/5] gve: clean XDP queues in gve_tx_stop_ring_gqi
    https://git.kernel.org/netdev/net/c/6321f5fb70d5
  - [net,2/5] gve: guard XDP xmit NDO on existence of xdp queues
    https://git.kernel.org/netdev/net/c/ff7c2dea9dd1
  - [net,3/5] gve: guard XSK operations on the existence of queues
    https://git.kernel.org/netdev/net/c/40338d7987d8
  - [net,4/5] gve: process XSK TX descriptors as part of RX NAPI
    https://git.kernel.org/netdev/net/c/ba0925c34e0f
  - [net,5/5] gve: fix XDP allocation path in edge cases
    https://git.kernel.org/netdev/net/c/de63ac44a527

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Joshua Washington]

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

I based usage of synchronize_net() instead of synchronize_rcu() based on other
drivers deciding to use it due to synchronize_rcu_expedited() when holding
rtnl_lock() being more performant.

ICE: https://lore.kernel.org/all/20240529112337.3639084-4-maciej.fijalkowski@intel.com/
Mellanox: https://lore.kernel.org/netdev/20210212025641.323844-8-saeed@kernel.org/

> 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.

Thank you for the suggestion. Given that the fix has gone in, I was planning
to make this change as part of a future net-next release with other XDP changes.
Would it make sense to make those changes there, given that the patches as
they went up, while not completely correct, should at least cover the
vulnerability?

Thanks,
Josh