netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used
@ 2021-06-18 10:52 Esben Haabendal
  2021-06-18 10:52 ` [PATCH 2/4] net: ll_temac: Add memory-barriers for TX BD access Esben Haabendal
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Esben Haabendal @ 2021-06-18 10:52 UTC (permalink / raw)
  To: netdev
  Cc: stable, David S. Miller, Jakub Kicinski, Michal Simek,
	Jesse Brandeburg, Wang Hai, Andrew Lunn, Zhang Changzhong,
	Michael Walle, linux-arm-kernel, linux-kernel

With the skb pointer piggy-backed on the TX BD, we have a simple and
efficient way to free the skb buffer when the frame has been transmitted.
But in order to avoid freeing the skb while there are still fragments from
the skb in use, we need to piggy-back on the TX BD of the skb, not the
first.

Without this, we are doing use-after-free on the DMA side, when the first
BD of a multi TX BD packet is seen as completed in xmit_done, and the
remaining BDs are still being processed.

Cc: stable@vger.kernel.org # v5.4+
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/net/ethernet/xilinx/ll_temac_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index a1f5f07f4ca9..e82f162cd80c 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -876,7 +876,6 @@ temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_OK;
 	}
 	cur_p->phys = cpu_to_be32(skb_dma_addr);
-	ptr_to_txbd((void *)skb, cur_p);
 
 	for (ii = 0; ii < num_frag; ii++) {
 		if (++lp->tx_bd_tail >= lp->tx_bd_num)
@@ -915,6 +914,11 @@ temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	}
 	cur_p->app0 |= cpu_to_be32(STS_CTRL_APP0_EOP);
 
+	/* Mark last fragment with skb address, so it can be consumed
+	 * in temac_start_xmit_done()
+	 */
+	ptr_to_txbd((void *)skb, cur_p);
+
 	tail_p = lp->tx_bd_p + sizeof(*lp->tx_bd_v) * lp->tx_bd_tail;
 	lp->tx_bd_tail++;
 	if (lp->tx_bd_tail >= lp->tx_bd_num)
-- 
2.32.0


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

* [PATCH 2/4] net: ll_temac: Add memory-barriers for TX BD access
  2021-06-18 10:52 [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used Esben Haabendal
@ 2021-06-18 10:52 ` Esben Haabendal
  2021-06-18 10:52 ` [PATCH 3/4] net: ll_temac: Fix TX BD buffer overwrite Esben Haabendal
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Esben Haabendal @ 2021-06-18 10:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Michal Simek, Michael Walle,
	Jesse Brandeburg, Andrew Lunn, Wang Hai, Zhang Changzhong,
	linux-arm-kernel, linux-kernel

Add a couple of memory-barriers to ensure correct ordering of read/write
access to TX BDs.

In xmit_done, we should ensure that reading the additional BD fields are
only done after STS_CTRL_APP0_CMPLT bit is set.

When xmit_done marks the BD as free by setting APP0=0, we need to ensure
that the other BD fields are reset first, so we avoid racing with the xmit
path, which writes to the same fields.

Finally, making sure to read APP0 of next BD after the current BD, ensures
that we see all available buffers.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/net/ethernet/xilinx/ll_temac_main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index e82f162cd80c..9797aa3221d1 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -774,12 +774,15 @@ static void temac_start_xmit_done(struct net_device *ndev)
 	stat = be32_to_cpu(cur_p->app0);
 
 	while (stat & STS_CTRL_APP0_CMPLT) {
+		/* Make sure that the other fields are read after bd is
+		 * released by dma
+		 */
+		rmb();
 		dma_unmap_single(ndev->dev.parent, be32_to_cpu(cur_p->phys),
 				 be32_to_cpu(cur_p->len), DMA_TO_DEVICE);
 		skb = (struct sk_buff *)ptr_from_txbd(cur_p);
 		if (skb)
 			dev_consume_skb_irq(skb);
-		cur_p->app0 = 0;
 		cur_p->app1 = 0;
 		cur_p->app2 = 0;
 		cur_p->app3 = 0;
@@ -788,6 +791,12 @@ static void temac_start_xmit_done(struct net_device *ndev)
 		ndev->stats.tx_packets++;
 		ndev->stats.tx_bytes += be32_to_cpu(cur_p->len);
 
+		/* app0 must be visible last, as it is used to flag
+		 * availability of the bd
+		 */
+		smp_mb();
+		cur_p->app0 = 0;
+
 		lp->tx_bd_ci++;
 		if (lp->tx_bd_ci >= lp->tx_bd_num)
 			lp->tx_bd_ci = 0;
@@ -814,6 +823,9 @@ static inline int temac_check_tx_bd_space(struct temac_local *lp, int num_frag)
 		if (cur_p->app0)
 			return NETDEV_TX_BUSY;
 
+		/* Make sure to read next bd app0 after this one */
+		rmb();
+
 		tail++;
 		if (tail >= lp->tx_bd_num)
 			tail = 0;
-- 
2.32.0


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

* [PATCH 3/4] net: ll_temac: Fix TX BD buffer overwrite
  2021-06-18 10:52 [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used Esben Haabendal
  2021-06-18 10:52 ` [PATCH 2/4] net: ll_temac: Add memory-barriers for TX BD access Esben Haabendal
@ 2021-06-18 10:52 ` Esben Haabendal
  2021-06-18 10:52 ` [PATCH 4/4] net: ll_temac: Avoid ndo_start_xmit returning NETDEV_TX_BUSY Esben Haabendal
  2021-06-18 19:20 ` [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: Esben Haabendal @ 2021-06-18 10:52 UTC (permalink / raw)
  To: netdev
  Cc: stable, David S. Miller, Jakub Kicinski, Michal Simek,
	Zhang Changzhong, Jesse Brandeburg, Andrew Lunn, Wang Hai,
	Michael Walle, linux-arm-kernel, linux-kernel

Just as the initial check, we need to ensure num_frag+1 buffers available,
as that is the number of buffers we are going to use.

This fixes a buffer overflow, which might be seen during heavy network
load. Complete lockup of TEMAC was reproducible within about 10 minutes of
a particular load.

Fixes: 84823ff80f74 ("net: ll_temac: Fix race condition causing TX hang")
Cc: stable@vger.kernel.org # v5.4+
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index 9797aa3221d1..cc482ee36501 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -861,7 +861,7 @@ temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		smp_mb();
 
 		/* Space might have just been freed - check again */
-		if (temac_check_tx_bd_space(lp, num_frag))
+		if (temac_check_tx_bd_space(lp, num_frag + 1))
 			return NETDEV_TX_BUSY;
 
 		netif_wake_queue(ndev);
-- 
2.32.0


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

* [PATCH 4/4] net: ll_temac: Avoid ndo_start_xmit returning NETDEV_TX_BUSY
  2021-06-18 10:52 [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used Esben Haabendal
  2021-06-18 10:52 ` [PATCH 2/4] net: ll_temac: Add memory-barriers for TX BD access Esben Haabendal
  2021-06-18 10:52 ` [PATCH 3/4] net: ll_temac: Fix TX BD buffer overwrite Esben Haabendal
@ 2021-06-18 10:52 ` Esben Haabendal
  2021-06-18 19:20 ` [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: Esben Haabendal @ 2021-06-18 10:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Michal Simek, Zhang Changzhong,
	Andrew Lunn, Michael Walle, Wang Hai, Jesse Brandeburg,
	linux-arm-kernel, linux-kernel

As documented in Documentation/networking/driver.rst, the ndo_start_xmit
method must not return NETDEV_TX_BUSY under any normal circumstances, and
as recommended, we simply stop the tx queue in advance, when there is a
risk that the next xmit would cause a NETDEV_TX_BUSY return.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/net/ethernet/xilinx/ll_temac_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index cc482ee36501..9a13953ea70f 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -942,6 +942,11 @@ temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	wmb();
 	lp->dma_out(lp, TX_TAILDESC_PTR, tail_p); /* DMA start */
 
+	if (temac_check_tx_bd_space(lp, MAX_SKB_FRAGS + 1)) {
+		netdev_info(ndev, "%s -> netif_stop_queue\n", __func__);
+		netif_stop_queue(ndev);
+	}
+
 	return NETDEV_TX_OK;
 }
 
-- 
2.32.0


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

* Re: [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used
  2021-06-18 10:52 [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used Esben Haabendal
                   ` (2 preceding siblings ...)
  2021-06-18 10:52 ` [PATCH 4/4] net: ll_temac: Avoid ndo_start_xmit returning NETDEV_TX_BUSY Esben Haabendal
@ 2021-06-18 19:20 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-06-18 19:20 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: netdev, stable, davem, kuba, michal.simek, jesse.brandeburg,
	wanghai38, andrew, zhangchangzhong, michael, linux-arm-kernel,
	linux-kernel

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Fri, 18 Jun 2021 12:52:23 +0200 you wrote:
> With the skb pointer piggy-backed on the TX BD, we have a simple and
> efficient way to free the skb buffer when the frame has been transmitted.
> But in order to avoid freeing the skb while there are still fragments from
> the skb in use, we need to piggy-back on the TX BD of the skb, not the
> first.
> 
> Without this, we are doing use-after-free on the DMA side, when the first
> BD of a multi TX BD packet is seen as completed in xmit_done, and the
> remaining BDs are still being processed.
> 
> [...]

Here is the summary with links:
  - [1/4] net: ll_temac: Make sure to free skb when it is completely used
    https://git.kernel.org/netdev/net/c/6aa32217a9a4
  - [2/4] net: ll_temac: Add memory-barriers for TX BD access
    https://git.kernel.org/netdev/net/c/28d9fab458b1
  - [3/4] net: ll_temac: Fix TX BD buffer overwrite
    https://git.kernel.org/netdev/net/c/c364df2489b8
  - [4/4] net: ll_temac: Avoid ndo_start_xmit returning NETDEV_TX_BUSY
    https://git.kernel.org/netdev/net/c/f63963411942

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] 5+ messages in thread

end of thread, other threads:[~2021-06-18 19:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 10:52 [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used Esben Haabendal
2021-06-18 10:52 ` [PATCH 2/4] net: ll_temac: Add memory-barriers for TX BD access Esben Haabendal
2021-06-18 10:52 ` [PATCH 3/4] net: ll_temac: Fix TX BD buffer overwrite Esben Haabendal
2021-06-18 10:52 ` [PATCH 4/4] net: ll_temac: Avoid ndo_start_xmit returning NETDEV_TX_BUSY Esben Haabendal
2021-06-18 19:20 ` [PATCH 1/4] net: ll_temac: Make sure to free skb when it is completely used patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).