| Message ID | 20241205091830.3719609-1-0x1207@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] net: stmmac: TSO: Fix unaligned DMA unmap for non-paged SKB data | expand |
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
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 > >
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.
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.
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/
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;
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(-)