| Message ID | 20230126101607.88407-1-jsc@umbraculum.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: ll_temac: fix DMA resources leak | expand |
On Thu, 26 Jan 2023 11:16:06 +0100 Jonas Suhr Christensen wrote: > Add missing conversion of address when unmapping dma region causing > unmapping to silently fail. At some point resulting in buffer > overrun eg. when releasing device. Could you add a Fixes tag pointing to the commit which introduced the bug? It will help the stable teams backport the patch. When reposting please put [PATCH net v2] as the prefix (noting the target tree for the benefit of bots/CIs). > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> > --- > drivers/net/ethernet/xilinx/ll_temac_main.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > index 1066420d6a83..66c04027f230 100644 > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > @@ -300,6 +300,7 @@ static void temac_dma_bd_release(struct net_device *ndev) > { > struct temac_local *lp = netdev_priv(ndev); > int i; > + struct cdmac_bd *bd; nit: we like variable declarations longest to shortest in networking so before the int i; pls > /* Reset Local Link (DMA) */ > lp->dma_out(lp, DMA_CONTROL_REG, DMA_CONTROL_RST);
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 26 Jan 2023 11:16:06 +0100 Jonas Suhr Christensen wrote: >> Add missing conversion of address when unmapping dma region causing >> unmapping to silently fail. At some point resulting in buffer >> overrun eg. when releasing device. > > Could you add a Fixes tag pointing to the commit which introduced > the bug? It will help the stable teams backport the patch. Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") > When reposting please put [PATCH net v2] as the prefix (noting > the target tree for the benefit of bots/CIs). > >> Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> >> --- >> drivers/net/ethernet/xilinx/ll_temac_main.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c >> index 1066420d6a83..66c04027f230 100644 >> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c >> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c >> @@ -300,6 +300,7 @@ static void temac_dma_bd_release(struct net_device *ndev) >> { >> struct temac_local *lp = netdev_priv(ndev); >> int i; >> + struct cdmac_bd *bd; > > nit: we like variable declarations longest to shortest in networking > so before the int i; pls > >> /* Reset Local Link (DMA) */ >> lp->dma_out(lp, DMA_CONTROL_REG, DMA_CONTROL_RST);
Le 26/01/2023 à 11:16, Jonas Suhr Christensen a écrit : > Add missing conversion of address when unmapping dma region causing > unmapping to silently fail. At some point resulting in buffer > overrun eg. when releasing device. > > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> > --- > drivers/net/ethernet/xilinx/ll_temac_main.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > index 1066420d6a83..66c04027f230 100644 > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > @@ -300,6 +300,7 @@ static void temac_dma_bd_release(struct net_device *ndev) > { > struct temac_local *lp = netdev_priv(ndev); > int i; > + struct cdmac_bd *bd; > > /* Reset Local Link (DMA) */ > lp->dma_out(lp, DMA_CONTROL_REG, DMA_CONTROL_RST); > @@ -307,9 +308,14 @@ static void temac_dma_bd_release(struct net_device *ndev) > for (i = 0; i < lp->rx_bd_num; i++) { > if (!lp->rx_skb[i]) > break; > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > + > + bd = &lp->rx_bd_v[1]; Hi, just a naive question from s.o. who knows nothing of this code: Is really [1] ([one]) expected here? [i] would look more "standard" in a 'for' loop. just my 2c, CJ > + dma_unmap_single(ndev->dev.parent, be32_to_cpu(bd->phys), > XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE); > + bd->phys = 0; > + bd->len = 0; > dev_kfree_skb(lp->rx_skb[i]); > + lp->rx_skb[i] = NULL; > } > if (lp->rx_bd_v) > dma_free_coherent(ndev->dev.parent,
On Mon, 30 Jan 2023 22:56:04 +0100 Christophe JAILLET wrote: > > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > > + > > + bd = &lp->rx_bd_v[1]; > > Hi, > just a naive question from s.o. who knows nothing of this code: > > Is really [1] ([one]) expected here? > [i] would look more "standard" in a 'for' loop. Wow, good eye.
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c index 1066420d6a83..66c04027f230 100644 --- a/drivers/net/ethernet/xilinx/ll_temac_main.c +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c @@ -300,6 +300,7 @@ static void temac_dma_bd_release(struct net_device *ndev) { struct temac_local *lp = netdev_priv(ndev); int i; + struct cdmac_bd *bd; /* Reset Local Link (DMA) */ lp->dma_out(lp, DMA_CONTROL_REG, DMA_CONTROL_RST); @@ -307,9 +308,14 @@ static void temac_dma_bd_release(struct net_device *ndev) for (i = 0; i < lp->rx_bd_num; i++) { if (!lp->rx_skb[i]) break; - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, + + bd = &lp->rx_bd_v[1]; + dma_unmap_single(ndev->dev.parent, be32_to_cpu(bd->phys), XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE); + bd->phys = 0; + bd->len = 0; dev_kfree_skb(lp->rx_skb[i]); + lp->rx_skb[i] = NULL; } if (lp->rx_bd_v) dma_free_coherent(ndev->dev.parent,
Add missing conversion of address when unmapping dma region causing unmapping to silently fail. At some point resulting in buffer overrun eg. when releasing device. Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> --- drivers/net/ethernet/xilinx/ll_temac_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)