[PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Furong Xu]

Commit 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for
non-paged SKB data") assigns a wrong DMA buffer address that is added an
offset of proto_hdr_len to tx_q->tx_skbuff_dma[entry].buf on a certain
platform that the DMA AXI address width is configured to 40-bit/48-bit,
stmmac_tx_clean() will try to unmap this illegal DMA buffer address
and many crashes are reported: [1] [2].

This patch guarantees that DMA address is passed to stmmac_tx_clean()
unmodified and without offset.

[1] https://lore.kernel.org/all/d8112193-0386-4e14-b516-37c2d838171a@nvidia.com/
[2] https://lore.kernel.org/all/klkzp5yn5kq5efgtrow6wbvnc46bcqfxs65nz3qy77ujr5turc@bwwhelz2l4dw/

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Reported-by: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Fixes: 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for non-paged SKB data")
Signed-off-by: Furong Xu <0x1207@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9b262cdad60b..7227f8428b5e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4192,8 +4192,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct stmmac_txq_stats *txq_stats;
 	struct stmmac_tx_queue *tx_q;
 	u32 pay_len, mss, queue;
+	dma_addr_t tso_hdr, des;
 	u8 proto_hdr_len, hdr;
-	dma_addr_t des;
 	bool set_ic;
 	int i;
 
@@ -4279,6 +4279,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 			     DMA_TO_DEVICE);
 	if (dma_mapping_error(priv->device, des))
 		goto dma_map_err;
+	tso_hdr = des;
 
 	if (priv->dma_cap.addr64 <= 32) {
 		first->des0 = cpu_to_le32(des);
@@ -4310,7 +4311,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * this DMA buffer right after the DMA engine completely finishes the
 	 * full buffer transmission.
 	 */
-	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = des;
+	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = tso_hdr;
 	tx_q->tx_skbuff_dma[tx_q->cur_tx].len = skb_headlen(skb);
 	tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = false;
 	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;
-- 
2.34.1

Re: [PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Jon Hunter]

On 05/12/2024 09:18, Furong Xu wrote:
> Commit 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for
> non-paged SKB data") assigns a wrong DMA buffer address that is added an
> offset of proto_hdr_len to tx_q->tx_skbuff_dma[entry].buf on a certain
> platform that the DMA AXI address width is configured to 40-bit/48-bit,

In our case it is 40/40 ...

  dwc-eth-dwmac 2490000.ethernet: Using 40/40 bits DMA host/device width

> stmmac_tx_clean() will try to unmap this illegal DMA buffer address
> and many crashes are reported: [1] [2].
> 
> This patch guarantees that DMA address is passed to stmmac_tx_clean()
> unmodified and without offset.
> 
> [1] https://lore.kernel.org/all/d8112193-0386-4e14-b516-37c2d838171a@nvidia.com/
> [2] https://lore.kernel.org/all/klkzp5yn5kq5efgtrow6wbvnc46bcqfxs65nz3qy77ujr5turc@bwwhelz2l4dw/
> 
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Reported-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> Fixes: 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for non-paged SKB data")
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9b262cdad60b..7227f8428b5e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4192,8 +4192,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>   	struct stmmac_txq_stats *txq_stats;
>   	struct stmmac_tx_queue *tx_q;
>   	u32 pay_len, mss, queue;
> +	dma_addr_t tso_hdr, des;
>   	u8 proto_hdr_len, hdr;
> -	dma_addr_t des;
>   	bool set_ic;
>   	int i;
>   
> @@ -4279,6 +4279,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>   			     DMA_TO_DEVICE);
>   	if (dma_mapping_error(priv->device, des))
>   		goto dma_map_err;
> +	tso_hdr = des;
>   
>   	if (priv->dma_cap.addr64 <= 32) {
>   		first->des0 = cpu_to_le32(des);
> @@ -4310,7 +4311,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>   	 * this DMA buffer right after the DMA engine completely finishes the
>   	 * full buffer transmission.
>   	 */
> -	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = des;
> +	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = tso_hdr;
>   	tx_q->tx_skbuff_dma[tx_q->cur_tx].len = skb_headlen(skb);
>   	tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = false;
>   	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;

Otherwise ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Jon

-- 
nvpublic

Re: [PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Russell King (Oracle)]

I'm slightly disappointed to have my patch turned into a commit under
someone else's authorship before I've had a chance to do that myself.
Next time I won't send a patch out until I've done that.

On Thu, Dec 05, 2024 at 05:18:30PM +0800, Furong Xu wrote:
> Commit 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for
> non-paged SKB data") assigns a wrong DMA buffer address that is added an
> offset of proto_hdr_len to tx_q->tx_skbuff_dma[entry].buf on a certain
> platform that the DMA AXI address width is configured to 40-bit/48-bit,
> stmmac_tx_clean() will try to unmap this illegal DMA buffer address
> and many crashes are reported: [1] [2].

This should mention that the DMA mapping API requires the cookie that is
returned from dma_map_single() be passed in unaltered to
dma_unmap_single(), and this driver does not do that when the DMA
address width is greater than 32-bit.

> 
> This patch guarantees that DMA address is passed to stmmac_tx_clean()
> unmodified and without offset.
> 
> [1] https://lore.kernel.org/all/d8112193-0386-4e14-b516-37c2d838171a@nvidia.com/
> [2] https://lore.kernel.org/all/klkzp5yn5kq5efgtrow6wbvnc46bcqfxs65nz3qy77ujr5turc@bwwhelz2l4dw/
> 
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Reported-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>

Please use rmk+kernel@armlinux.org.uk there.

> Fixes: 66600fac7a98 ("net: stmmac: TSO: Fix unbalanced DMA map/unmap for non-paged SKB data")
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9b262cdad60b..7227f8428b5e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4192,8 +4192,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct stmmac_txq_stats *txq_stats;
>  	struct stmmac_tx_queue *tx_q;
>  	u32 pay_len, mss, queue;
> +	dma_addr_t tso_hdr, des;
>  	u8 proto_hdr_len, hdr;
> -	dma_addr_t des;
>  	bool set_ic;
>  	int i;
>  
> @@ -4279,6 +4279,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  			     DMA_TO_DEVICE);
>  	if (dma_mapping_error(priv->device, des))
>  		goto dma_map_err;
> +	tso_hdr = des;
>  
>  	if (priv->dma_cap.addr64 <= 32) {
>  		first->des0 = cpu_to_le32(des);
> @@ -4310,7 +4311,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 * this DMA buffer right after the DMA engine completely finishes the
>  	 * full buffer transmission.
>  	 */
> -	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = des;
> +	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = tso_hdr;
>  	tx_q->tx_skbuff_dma[tx_q->cur_tx].len = skb_headlen(skb);
>  	tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = false;
>  	tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;
> -- 
> 2.34.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Jakub Kicinski]

On Thu, 5 Dec 2024 16:43:22 +0000 Russell King (Oracle) wrote:
> I'm slightly disappointed to have my patch turned into a commit under
> someone else's authorship before I've had a chance to do that myself.
> Next time I won't send a patch out until I've done that.

Yes, this is definitely not okay. LMK if you dropped this from your
TODO already, otherwise I'm tossing this patch and expecting the fix
from the real author.

Side rant - the Suggested-by tag is completely meaningless, maybe we
should stop using it. The usage ranges from crediting people pointing
out issues in basic code review, to crediting authors when stealing
their code. What is the point.

Re: [PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Russell King (Oracle)]

On Thu, Dec 05, 2024 at 08:55:39AM -0800, Jakub Kicinski wrote:
> On Thu, 5 Dec 2024 16:43:22 +0000 Russell King (Oracle) wrote:
> > I'm slightly disappointed to have my patch turned into a commit under
> > someone else's authorship before I've had a chance to do that myself.
> > Next time I won't send a patch out until I've done that.
> 
> Yes, this is definitely not okay. LMK if you dropped this from your
> TODO already, otherwise I'm tossing this patch and expecting the fix
> from the real author.

See https://lore.kernel.org/r/E1tJXcx-006N4Z-PC@rmk-PC.armlinux.org.uk/

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data

[Furong Xu]

Hi Russell,

On Thu, 5 Dec 2024 16:43:22 +0000, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> I'm slightly disappointed to have my patch turned into a commit under
> someone else's authorship before I've had a chance to do that myself.
> Next time I won't send a patch out until I've done that.
> 
I am really sorry for this, I should have requested your permission first.

> 
> Please use rmk+kernel@armlinux.org.uk there.
> 
So another iteration is required here.
Would you mind me send the v2 of this fix?
I will not send v2 without your permission.

Thanks.