[PATCH] net: renesas: sh_eth: Fix freeing wrong tx descriptor

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.
Otherwise NETDEV WATCHDOG timeout is possible to happen.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 6c8ba916d1a6..1374faa229a2 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c

@@ -2533,6 +2533,7 @@ static netdev_tx_t sh_eth_start_xmit(struct sk_buff *skb,
 	else
 		txdesc->status |= cpu_to_le32(TD_TACT);
 
+	wmb(); /* cur_tx must be incremented after TACT bit was set */
 	mdp->cur_tx++;
 
 	if (!(sh_eth_read(ndev, EDTRR) & mdp->cd->edtrr_trns))

-- 
2.25.1

Hello:

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

On Tue,  7 Sep 2021 20:29:40 +0900 you wrote:

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.
Otherwise NETDEV WATCHDOG timeout is possible to happen.

[...]

Here is the summary with links:
  - net: renesas: sh_eth: Fix freeing wrong tx descriptor
    https://git.kernel.org/netdev/net/c/0341d5e3d1ee

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

On 9/7/21 2:29 PM, Yoshihiro Shimoda wrote:

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.

   Not dma_wmb()? :-)

Otherwise NETDEV WATCHDOG timeout is possible to happen.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Sergey Shtylyov <redacted>

[...]

MBR, Sergey

Hi Sergey,

From: Sergey Shtylyov, Sent: Wednesday, September 8, 2021 4:30 AM

On 9/7/21 2:29 PM, Yoshihiro Shimoda wrote:

quoted

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.

   Not dma_wmb()? :-)

On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST.
IIUC, DMB OSHST is not affected the ordering of instructions.
So, we have to use wmb().

quoted

Otherwise NETDEV WATCHDOG timeout is possible to happen.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Sergey Shtylyov <redacted>

Thank you for your review!

Best regards,
Yoshihiro Shimoda

On 08.09.2021 8:45, Yoshihiro Shimoda wrote:

quoted

quoted

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.

    Not dma_wmb()? :-)

On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST.
IIUC, DMB OSHST is not affected the ordering of instructions.
So, we have to use wmb().

    I should really read up the ARM manuals on the barrier instructions... :-)

quoted

quoted

Otherwise NETDEV WATCHDOG timeout is possible to happen.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Sergey Shtylyov <redacted>

Thank you for your review!

    Out of curiosity: have you really experienced the bug or found it by
review?

Best regards,
Yoshihiro Shimoda

MBR, Sergey

Hi Sergey,

From: Sergey Shtylyov, Sent: Wednesday, September 8, 2021 6:46 PM

On 08.09.2021 8:45, Yoshihiro Shimoda wrote:

quoted

quoted

quoted

The cur_tx counter must be incremented after TACT bit of
txdesc->status was set. However, a CPU is possible to reorder
instructions and/or memory accesses between cur_tx and
txdesc->status. And then, if TX interrupt happened at such a
timing, the sh_eth_tx_free() may free the descriptor wrongly.
So, add wmb() before cur_tx++.

    Not dma_wmb()? :-)

On armv8, dma_wmb() is DMB OSHST, and wmb() is DSB ST.
IIUC, DMB OSHST is not affected the ordering of instructions.
So, we have to use wmb().

    I should really read up the ARM manuals on the barrier instructions... :-)

:)

quoted

quoted

quoted

Otherwise NETDEV WATCHDOG timeout is possible to happen.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Sergey Shtylyov <redacted>

Thank you for your review!

    Out of curiosity: have you really experienced the bug or found it by
review?

Our team has really experienced the bug when iperf3 runs on both side(server and client),
and this patch could fix the issue.

Best regards,
Yoshihiro Shimoda

quoted

Best regards,
Yoshihiro Shimoda

MBR, Sergey