All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done()
@ 2023-01-25  7:48 Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 1/5] qede: " Magnus Karlsson
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:48 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: Magnus Karlsson, bpf

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found in [1].

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in [2].

The drivers have only been compile-tested since I do not own any of
the HW below. So if you are a maintainer, it would be great if you
could take a quick look to make sure I did not mess something up.

Note that these were the drivers I found that violated the ordering by
running a simple script and manually checking the ones that came up as
potential offenders. But the script was not perfect in any way. There
might still be offenders out there, since the script can generate
false negatives.

v1 -> v2:
* Added acks [Toke, Steen]
* Corrected two spelling errors [Toke]

[1] https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
[2] https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/

Thanks: Magnus

Magnus Karlsson (5):
  qede: execute xdp_do_flush() before napi_complete_done()
  lan966x: execute xdp_do_flush() before napi_complete_done()
  virtio-net: execute xdp_do_flush() before napi_complete_done()
  dpaa_eth: execute xdp_do_flush() before napi_complete_done()
  dpaa2-eth: execute xdp_do_flush() before napi_complete_done()

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        | 6 +++---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c      | 9 ++++++---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 6 +++---
 drivers/net/ethernet/qlogic/qede/qede_fp.c            | 7 ++++---
 drivers/net/virtio_net.c                              | 6 +++---
 5 files changed, 19 insertions(+), 15 deletions(-)


base-commit: 2a48216cff7a2e3964fbed16f84d33f68b3e5e42
--
2.34.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net v2 1/5] qede: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
@ 2023-01-25  7:48 ` Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 2/5] lan966x: " Magnus Karlsson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:48 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: bpf

From: Magnus Karlsson <magnus.karlsson@intel.com>

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found following the first link below.

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in the second link below.

Fixes: d1b25b79e162b ("qede: add .ndo_xdp_xmit() and XDP_REDIRECT support")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
---
 drivers/net/ethernet/qlogic/qede/qede_fp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
index 7c2af482192d..cb1746bc0e0c 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
@@ -1438,6 +1438,10 @@ int qede_poll(struct napi_struct *napi, int budget)
 	rx_work_done = (likely(fp->type & QEDE_FASTPATH_RX) &&
 			qede_has_rx_work(fp->rxq)) ?
 			qede_rx_int(fp, budget) : 0;
+
+	if (fp->xdp_xmit & QEDE_XDP_REDIRECT)
+		xdp_do_flush();
+
 	/* Handle case where we are called by netpoll with a budget of 0 */
 	if (rx_work_done < budget || !budget) {
 		if (!qede_poll_is_more_work(fp)) {
@@ -1457,9 +1461,6 @@ int qede_poll(struct napi_struct *napi, int budget)
 		qede_update_tx_producer(fp->xdp_tx);
 	}
 
-	if (fp->xdp_xmit & QEDE_XDP_REDIRECT)
-		xdp_do_flush_map();
-
 	return rx_work_done;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v2 2/5] lan966x: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 1/5] qede: " Magnus Karlsson
@ 2023-01-25  7:48 ` Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 3/5] virtio-net: " Magnus Karlsson
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:48 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: bpf, Steen Hegelund

From: Magnus Karlsson <magnus.karlsson@intel.com>

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found following the first link below.

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in the second link below.

Fixes: a825b611c7c1 ("net: lan966x: Add support for XDP_REDIRECT")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 5314c064ceae..55b484b10562 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -608,12 +608,12 @@ static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
 		lan966x_fdma_rx_reload(rx);
 	}
 
-	if (counter < weight && napi_complete_done(napi, counter))
-		lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
-
 	if (redirect)
 		xdp_do_flush();
 
+	if (counter < weight && napi_complete_done(napi, counter))
+		lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
+
 	return counter;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v2 3/5] virtio-net: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 1/5] qede: " Magnus Karlsson
  2023-01-25  7:48 ` [PATCH net v2 2/5] lan966x: " Magnus Karlsson
@ 2023-01-25  7:48 ` Magnus Karlsson
  2023-01-27 10:49   ` Michael S. Tsirkin
  2023-01-25  7:49 ` [PATCH net v2 4/5] dpaa_eth: " Magnus Karlsson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:48 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: bpf

From: Magnus Karlsson <magnus.karlsson@intel.com>

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found following the first link below.

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in the second link below.

Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
---
 drivers/net/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 18b3de854aeb..6df14dd5bf46 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1677,13 +1677,13 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 
 	received = virtnet_receive(rq, budget, &xdp_xmit);
 
+	if (xdp_xmit & VIRTIO_XDP_REDIR)
+		xdp_do_flush();
+
 	/* Out of packets? */
 	if (received < budget)
 		virtqueue_napi_complete(napi, rq->vq, received);
 
-	if (xdp_xmit & VIRTIO_XDP_REDIR)
-		xdp_do_flush();
-
 	if (xdp_xmit & VIRTIO_XDP_TX) {
 		sq = virtnet_xdp_get_sq(vi);
 		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v2 4/5] dpaa_eth: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
                   ` (2 preceding siblings ...)
  2023-01-25  7:48 ` [PATCH net v2 3/5] virtio-net: " Magnus Karlsson
@ 2023-01-25  7:49 ` Magnus Karlsson
  2023-01-25 14:58   ` Camelia Alexandra Groza
  2023-01-25  7:49 ` [PATCH net v2 5/5] dpaa2-eth: " Magnus Karlsson
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:49 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: bpf

From: Magnus Karlsson <magnus.karlsson@intel.com>

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found following the first link below.

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in the second link below.

Fixes: a1e031ffb422 ("dpaa_eth: add XDP_REDIRECT support")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 3f8032947d86..027fff9f7db0 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2410,6 +2410,9 @@ static int dpaa_eth_poll(struct napi_struct *napi, int budget)
 
 	cleaned = qman_p_poll_dqrr(np->p, budget);
 
+	if (np->xdp_act & XDP_REDIRECT)
+		xdp_do_flush();
+
 	if (cleaned < budget) {
 		napi_complete_done(napi, cleaned);
 		qman_p_irqsource_add(np->p, QM_PIRQ_DQRI);
@@ -2417,9 +2420,6 @@ static int dpaa_eth_poll(struct napi_struct *napi, int budget)
 		qman_p_irqsource_add(np->p, QM_PIRQ_DQRI);
 	}
 
-	if (np->xdp_act & XDP_REDIRECT)
-		xdp_do_flush();
-
 	return cleaned;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v2 5/5] dpaa2-eth: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
                   ` (3 preceding siblings ...)
  2023-01-25  7:49 ` [PATCH net v2 4/5] dpaa_eth: " Magnus Karlsson
@ 2023-01-25  7:49 ` Magnus Karlsson
  2023-01-27 10:50 ` [PATCH net v2 0/5] net: xdp: " Michael S. Tsirkin
  2023-01-28  6:40 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: Magnus Karlsson @ 2023-01-25  7:49 UTC (permalink / raw)
  To: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur
  Cc: bpf

From: Magnus Karlsson <magnus.karlsson@intel.com>

Make sure that xdp_do_flush() is always executed before
napi_complete_done(). This is important for two reasons. First, a
redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
napi context X on CPU Y will be followed by a xdp_do_flush() from the
same napi context and CPU. This is not guaranteed if the
napi_complete_done() is executed before xdp_do_flush(), as it tells
the napi logic that it is fine to schedule napi context X on another
CPU. Details from a production system triggering this bug using the
veth driver can be found following the first link below.

The second reason is that the XDP_REDIRECT logic in itself relies on
being inside a single NAPI instance through to the xdp_do_flush() call
for RCU protection of all in-kernel data structures. Details can be
found in the second link below.

Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 0c35abb7d065..2e79d18fc3c7 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1993,10 +1993,15 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget)
 		if (rx_cleaned >= budget ||
 		    txconf_cleaned >= DPAA2_ETH_TXCONF_PER_NAPI) {
 			work_done = budget;
+			if (ch->xdp.res & XDP_REDIRECT)
+				xdp_do_flush();
 			goto out;
 		}
 	} while (store_cleaned);
 
+	if (ch->xdp.res & XDP_REDIRECT)
+		xdp_do_flush();
+
 	/* Update NET DIM with the values for this CDAN */
 	dpaa2_io_update_net_dim(ch->dpio, ch->stats.frames_per_cdan,
 				ch->stats.bytes_per_cdan);
@@ -2032,9 +2037,7 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget)
 		txc_fq->dq_bytes = 0;
 	}
 
-	if (ch->xdp.res & XDP_REDIRECT)
-		xdp_do_flush_map();
-	else if (rx_cleaned && ch->xdp.res & XDP_TX)
+	if (rx_cleaned && ch->xdp.res & XDP_TX)
 		dpaa2_eth_xdp_tx_flush(priv, ch, &priv->fq[flowid]);
 
 	return work_done;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH net v2 4/5] dpaa_eth: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:49 ` [PATCH net v2 4/5] dpaa_eth: " Magnus Karlsson
@ 2023-01-25 14:58   ` Camelia Alexandra Groza
  0 siblings, 0 replies; 10+ messages in thread
From: Camelia Alexandra Groza @ 2023-01-25 14:58 UTC (permalink / raw)
  To: Magnus Karlsson, magnus.karlsson, bjorn, ast, daniel, netdev,
	jonathan.lemon, maciej.fijalkowski, kuba, toke, pabeni, davem,
	aelior, manishc, horatiu.vultur, UNGLinuxDriver, mst, jasowang,
	Ioana Ciornei, Madalin Bucur
  Cc: bpf

> -----Original Message-----
> From: Magnus Karlsson <magnus.karlsson@gmail.com>
> Sent: Wednesday, January 25, 2023 9:49
> To: magnus.karlsson@intel.com; bjorn@kernel.org; ast@kernel.org;
> daniel@iogearbox.net; netdev@vger.kernel.org;
> jonathan.lemon@gmail.com; maciej.fijalkowski@intel.com;
> kuba@kernel.org; toke@redhat.com; pabeni@redhat.com;
> davem@davemloft.net; aelior@marvell.com; manishc@marvell.com;
> horatiu.vultur@microchip.com; UNGLinuxDriver@microchip.com;
> mst@redhat.com; jasowang@redhat.com; Ioana Ciornei
> <ioana.ciornei@nxp.com>; Madalin Bucur <madalin.bucur@nxp.com>
> Cc: bpf@vger.kernel.org
> Subject: [PATCH net v2 4/5] dpaa_eth: execute xdp_do_flush() before
> napi_complete_done()
> 
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Make sure that xdp_do_flush() is always executed before
> napi_complete_done(). This is important for two reasons. First, a
> redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
> napi context X on CPU Y will be followed by a xdp_do_flush() from the
> same napi context and CPU. This is not guaranteed if the
> napi_complete_done() is executed before xdp_do_flush(), as it tells
> the napi logic that it is fine to schedule napi context X on another
> CPU. Details from a production system triggering this bug using the
> veth driver can be found following the first link below.
> 
> The second reason is that the XDP_REDIRECT logic in itself relies on
> being inside a single NAPI instance through to the xdp_do_flush() call
> for RCU protection of all in-kernel data structures. Details can be
> found in the second link below.
> 
> Fixes: a1e031ffb422 ("dpaa_eth: add XDP_REDIRECT support")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
> Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
> ---

Acked-by: Camelia Groza <camelia.groza@nxp.com>

Thanks!

>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 3f8032947d86..027fff9f7db0 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2410,6 +2410,9 @@ static int dpaa_eth_poll(struct napi_struct *napi, int
> budget)
> 
>  	cleaned = qman_p_poll_dqrr(np->p, budget);
> 
> +	if (np->xdp_act & XDP_REDIRECT)
> +		xdp_do_flush();
> +
>  	if (cleaned < budget) {
>  		napi_complete_done(napi, cleaned);
>  		qman_p_irqsource_add(np->p, QM_PIRQ_DQRI);
> @@ -2417,9 +2420,6 @@ static int dpaa_eth_poll(struct napi_struct *napi, int
> budget)
>  		qman_p_irqsource_add(np->p, QM_PIRQ_DQRI);
>  	}
> 
> -	if (np->xdp_act & XDP_REDIRECT)
> -		xdp_do_flush();
> -
>  	return cleaned;
>  }
> 
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v2 3/5] virtio-net: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 ` [PATCH net v2 3/5] virtio-net: " Magnus Karlsson
@ 2023-01-27 10:49   ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-01-27 10:49 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, jasowang, ioana.ciornei,
	madalin.bucur, bpf

On Wed, Jan 25, 2023 at 08:48:59AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Make sure that xdp_do_flush() is always executed before
> napi_complete_done(). This is important for two reasons. First, a
> redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
> napi context X on CPU Y will be followed by a xdp_do_flush() from the
> same napi context and CPU. This is not guaranteed if the
> napi_complete_done() is executed before xdp_do_flush(), as it tells
> the napi logic that it is fine to schedule napi context X on another
> CPU. Details from a production system triggering this bug using the
> veth driver can be found following the first link below.
> 
> The second reason is that the XDP_REDIRECT logic in itself relies on
> being inside a single NAPI instance through to the xdp_do_flush() call
> for RCU protection of all in-kernel data structures. Details can be
> found in the second link below.
> 
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Link: https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
> Link: https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 18b3de854aeb..6df14dd5bf46 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1677,13 +1677,13 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>  
>  	received = virtnet_receive(rq, budget, &xdp_xmit);
>  
> +	if (xdp_xmit & VIRTIO_XDP_REDIR)
> +		xdp_do_flush();
> +
>  	/* Out of packets? */
>  	if (received < budget)
>  		virtqueue_napi_complete(napi, rq->vq, received);
>  
> -	if (xdp_xmit & VIRTIO_XDP_REDIR)
> -		xdp_do_flush();
> -
>  	if (xdp_xmit & VIRTIO_XDP_TX) {
>  		sq = virtnet_xdp_get_sq(vi);
>  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
> -- 
> 2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
                   ` (4 preceding siblings ...)
  2023-01-25  7:49 ` [PATCH net v2 5/5] dpaa2-eth: " Magnus Karlsson
@ 2023-01-27 10:50 ` Michael S. Tsirkin
  2023-01-28  6:40 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-01-27 10:50 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, jasowang, ioana.ciornei,
	madalin.bucur, bpf

On Wed, Jan 25, 2023 at 08:48:56AM +0100, Magnus Karlsson wrote:
> Make sure that xdp_do_flush() is always executed before
> napi_complete_done(). This is important for two reasons. First, a
> redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
> napi context X on CPU Y will be followed by a xdp_do_flush() from the
> same napi context and CPU. This is not guaranteed if the
> napi_complete_done() is executed before xdp_do_flush(), as it tells
> the napi logic that it is fine to schedule napi context X on another
> CPU. Details from a production system triggering this bug using the
> veth driver can be found in [1].
> 
> The second reason is that the XDP_REDIRECT logic in itself relies on
> being inside a single NAPI instance through to the xdp_do_flush() call
> for RCU protection of all in-kernel data structures. Details can be
> found in [2].
> 
> The drivers have only been compile-tested since I do not own any of
> the HW below. So if you are a maintainer, it would be great if you
> could take a quick look to make sure I did not mess something up.
> 
> Note that these were the drivers I found that violated the ordering by
> running a simple script and manually checking the ones that came up as
> potential offenders. But the script was not perfect in any way. There
> might still be offenders out there, since the script can generate
> false negatives.


BTW all this series is stable material, right?


> v1 -> v2:
> * Added acks [Toke, Steen]
> * Corrected two spelling errors [Toke]
> 
> [1] https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
> [2] https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
> 
> Thanks: Magnus
> 
> Magnus Karlsson (5):
>   qede: execute xdp_do_flush() before napi_complete_done()
>   lan966x: execute xdp_do_flush() before napi_complete_done()
>   virtio-net: execute xdp_do_flush() before napi_complete_done()
>   dpaa_eth: execute xdp_do_flush() before napi_complete_done()
>   dpaa2-eth: execute xdp_do_flush() before napi_complete_done()
> 
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        | 6 +++---
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c      | 9 ++++++---
>  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 6 +++---
>  drivers/net/ethernet/qlogic/qede/qede_fp.c            | 7 ++++---
>  drivers/net/virtio_net.c                              | 6 +++---
>  5 files changed, 19 insertions(+), 15 deletions(-)
> 
> 
> base-commit: 2a48216cff7a2e3964fbed16f84d33f68b3e5e42
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done()
  2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
                   ` (5 preceding siblings ...)
  2023-01-27 10:50 ` [PATCH net v2 0/5] net: xdp: " Michael S. Tsirkin
@ 2023-01-28  6:40 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-28  6:40 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: magnus.karlsson, bjorn, ast, daniel, netdev, jonathan.lemon,
	maciej.fijalkowski, kuba, toke, pabeni, davem, aelior, manishc,
	horatiu.vultur, UNGLinuxDriver, mst, jasowang, ioana.ciornei,
	madalin.bucur, bpf

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 25 Jan 2023 08:48:56 +0100 you wrote:
> Make sure that xdp_do_flush() is always executed before
> napi_complete_done(). This is important for two reasons. First, a
> redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
> napi context X on CPU Y will be followed by a xdp_do_flush() from the
> same napi context and CPU. This is not guaranteed if the
> napi_complete_done() is executed before xdp_do_flush(), as it tells
> the napi logic that it is fine to schedule napi context X on another
> CPU. Details from a production system triggering this bug using the
> veth driver can be found in [1].
> 
> [...]

Here is the summary with links:
  - [net,v2,1/5] qede: execute xdp_do_flush() before napi_complete_done()
    https://git.kernel.org/netdev/net/c/2ccce20d51fa
  - [net,v2,2/5] lan966x: execute xdp_do_flush() before napi_complete_done()
    https://git.kernel.org/netdev/net/c/12b5717990c8
  - [net,v2,3/5] virtio-net: execute xdp_do_flush() before napi_complete_done()
    https://git.kernel.org/netdev/net/c/ad7e615f646c
  - [net,v2,4/5] dpaa_eth: execute xdp_do_flush() before napi_complete_done()
    https://git.kernel.org/netdev/net/c/b534013798b7
  - [net,v2,5/5] dpaa2-eth: execute xdp_do_flush() before napi_complete_done()
    https://git.kernel.org/netdev/net/c/a3191c4d86c5

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-01-28  6:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-25  7:48 [PATCH net v2 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done() Magnus Karlsson
2023-01-25  7:48 ` [PATCH net v2 1/5] qede: " Magnus Karlsson
2023-01-25  7:48 ` [PATCH net v2 2/5] lan966x: " Magnus Karlsson
2023-01-25  7:48 ` [PATCH net v2 3/5] virtio-net: " Magnus Karlsson
2023-01-27 10:49   ` Michael S. Tsirkin
2023-01-25  7:49 ` [PATCH net v2 4/5] dpaa_eth: " Magnus Karlsson
2023-01-25 14:58   ` Camelia Alexandra Groza
2023-01-25  7:49 ` [PATCH net v2 5/5] dpaa2-eth: " Magnus Karlsson
2023-01-27 10:50 ` [PATCH net v2 0/5] net: xdp: " Michael S. Tsirkin
2023-01-28  6:40 ` patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.