| Message ID | 20230205201130.11303-2-jsc@umbraculum.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix dma leaking | expand |
> -----Original Message----- > From: Jonas Suhr Christensen <jsc@umbraculum.org> > Sent: Monday, February 6, 2023 1:41 AM > To: netdev@vger.kernel.org > Cc: jsc@umbraculum.org; David S. Miller <davem@davemloft.net>; Eric > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > Paolo Abeni <pabeni@redhat.com>; Michal Simek > <michal.simek@xilinx.com>; Katakam, Harini <harini.katakam@amd.com>; > Haoyue Xu <xuhaoyue1@hisilicon.com>; huangjunxian > <huangjunxian6@hisilicon.com>; Wang Qing <wangqing@vivo.com>; Yang > Yingliang <yangyingliang@huawei.com>; Esben Haabendal > <esben@geanix.com>; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak > > 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. > > Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") > > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> Thanks for the patch. Reviewed-by: Harini Katakam <harini.katakam@amd.com> Regards, Harini
Jonas Suhr Christensen <jsc@umbraculum.org> writes: > 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. > > Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") > > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> Acked-by: Esben Haabendal <esben@geanix.com>
Hi, On Sun, 2023-02-05 at 21:11 +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. > > Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") > > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> I'm sorry for nit-picking, but you must avoid empty lines in the tag area. Please post a v2 avoiding the empty line between the Fixes and sob tags (both here and in the next patch). You can retain (include in the tag area) the already collected reviewed-by/acked-by tags. Thanks, Paolo
On Tue, 2023-02-07 at 12:27 +0100, Paolo Abeni wrote: > Hi, > > On Sun, 2023-02-05 at 21:11 +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. > > > > Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") > > > > Signed-off-by: Jonas Suhr Christensen <jsc@umbraculum.org> > > I'm sorry for nit-picking, but you must avoid empty lines in the tag > area. Please post a v2 avoiding the empty line between the Fixes and > sob tags (both here and in the next patch). > > You can retain (include in the tag area) the already collected > reviewed-by/acked-by tags. I'm sorry, I'm low on coffee. I forgot to mention a more relevant thing: > @@ -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[i]; > + dma_unmap_single(ndev->dev.parent, be32_to_cpu(bd->phys), The above be32_to_cpu() introduces a new build warning. as phys type is u32. It looks like the existing code generates a lot of similar warns. You can either try change to phys type to __be32 (likely not suitable for -net and possibly can introduce even more warnings elsewhere) or explicitly cast the argument. Thanks, Paolo
On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: > You can either try change to phys type to __be32 (likely not suitable > for -net and possibly can introduce even more warnings elsewhere) FWIW that seems like the best option to me as well. Let's ignore the sparse warning for v3 and try to switch phys to __be32 in a separate patch for net-next. No point adding force casts just to have to remove them a week later, given how prevalent the problem is. > or explicitly cast the argument.
On Tue, Feb 7, 2023, at 19:42, Jakub Kicinski wrote: > On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: >> You can either try change to phys type to __be32 (likely not suitable >> for -net and possibly can introduce even more warnings elsewhere) > > FWIW that seems like the best option to me as well. Let's ignore the > sparse warning for v3 and try to switch phys to __be32 in a separate > patch for net-next. No point adding force casts just to have to remove > them a week later, given how prevalent the problem is. > >> or explicitly cast the argument. I no longer have access to the hardware, so I'm not rewriting the batch. Feel free to take ownership of it and fix what's needed.
On Mon, 13 Mar 2023 19:37:00 +0100 Jonas Suhr Christensen wrote: > On Tue, Feb 7, 2023, at 19:42, Jakub Kicinski wrote: > > On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: > >> You can either try change to phys type to __be32 (likely not suitable > >> for -net and possibly can introduce even more warnings elsewhere) > > > > FWIW that seems like the best option to me as well. Let's ignore the > > sparse warning for v3 and try to switch phys to __be32 in a separate > > patch for net-next. No point adding force casts just to have to remove > > them a week later, given how prevalent the problem is. > > > >> or explicitly cast the argument. > > I no longer have access to the hardware, so I'm not rewriting the > batch. Feel free to take ownership of it and fix what's needed. Ack. Harini, are you the designated maintainer for this driver? Could you add a MAINTAINERS entry for it? I don't see one right now. And possibly pick up these patches / fix the problem, if you have the cycles?
Hi Jakub, Jonas, > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, March 14, 2023 12:19 AM > To: Jonas Suhr Christensen <jsc@umbraculum.org>; Katakam, Harini > <harini.katakam@amd.com> > Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > Michal Simek <michal.simek@xilinx.com>; Haoyue Xu > <xuhaoyue1@hisilicon.com>; huangjunxian <huangjunxian6@hisilicon.com>; > Wang Qing <wangqing@vivo.com>; Yang Yingliang > <yangyingliang@huawei.com>; Esben Haabendal <esben@geanix.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak > > On Mon, 13 Mar 2023 19:37:00 +0100 Jonas Suhr Christensen wrote: > > On Tue, Feb 7, 2023, at 19:42, Jakub Kicinski wrote: > > > On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: > > >> You can either try change to phys type to __be32 (likely not > > >> suitable for -net and possibly can introduce even more warnings > > >> elsewhere) > > > > > > FWIW that seems like the best option to me as well. Let's ignore the > > > sparse warning for v3 and try to switch phys to __be32 in a separate > > > patch for net-next. No point adding force casts just to have to > > > remove them a week later, given how prevalent the problem is. > > > > > >> or explicitly cast the argument. > > > > I no longer have access to the hardware, so I'm not rewriting the > > batch. Feel free to take ownership of it and fix what's needed. > > Ack. > > Harini, are you the designated maintainer for this driver? Could you add a > MAINTAINERS entry for it? I don't see one right now. > And possibly pick up these patches / fix the problem, if you have the cycles? Sure, Srinivas (cced) will pick up this series and send a v3. I'll get back on the state of this IP/driver for the maintainers list. Will include that patch in the beginning of the series as well. Regards, Harini
Le 14/03/2023 à 06:15, Katakam, Harini a écrit : > Hi Jakub, Jonas, > >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: Tuesday, March 14, 2023 12:19 AM >> To: Jonas Suhr Christensen <jsc@umbraculum.org>; Katakam, Harini >> <harini.katakam@amd.com> >> Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. >> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; >> Michal Simek <michal.simek@xilinx.com>; Haoyue Xu >> <xuhaoyue1@hisilicon.com>; huangjunxian <huangjunxian6@hisilicon.com>; >> Wang Qing <wangqing@vivo.com>; Yang Yingliang >> <yangyingliang@huawei.com>; Esben Haabendal <esben@geanix.com>; >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak >> >> On Mon, 13 Mar 2023 19:37:00 +0100 Jonas Suhr Christensen wrote: >>> On Tue, Feb 7, 2023, at 19:42, Jakub Kicinski wrote: >>>> On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: >>>>> You can either try change to phys type to __be32 (likely not >>>>> suitable for -net and possibly can introduce even more warnings >>>>> elsewhere) >>>> >>>> FWIW that seems like the best option to me as well. Let's ignore the >>>> sparse warning for v3 and try to switch phys to __be32 in a separate >>>> patch for net-next. No point adding force casts just to have to >>>> remove them a week later, given how prevalent the problem is. >>>> >>>>> or explicitly cast the argument. >>> >>> I no longer have access to the hardware, so I'm not rewriting the >>> batch. Feel free to take ownership of it and fix what's needed. >> >> Ack. >> >> Harini, are you the designated maintainer for this driver? Could you add a >> MAINTAINERS entry for it? I don't see one right now. >> And possibly pick up these patches / fix the problem, if you have the cycles? > > Sure, Srinivas (cced) will pick up this series and send a v3. > I'll get back on the state of this IP/driver for the maintainers list. Will include > that patch in the beginning of the series as well. > > Regards, > Harini > Hi, this patch, or an updated version, has not reached -next yet. Does someone still working on it, or did it got lost? CJ
Hi Christophe, > -----Original Message----- > From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Sent: Saturday, July 8, 2023 6:46 PM > To: Katakam, Harini <harini.katakam@amd.com>; Jakub Kicinski > <kuba@kernel.org>; Jonas Suhr Christensen <jsc@umbraculum.org> > Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > Michal Simek <michal.simek@xilinx.com>; Haoyue Xu > <xuhaoyue1@hisilicon.com>; huangjunxian <huangjunxian6@hisilicon.com>; > Wang Qing <wangqing@vivo.com>; Yang Yingliang > <yangyingliang@huawei.com>; Esben Haabendal <esben@geanix.com>; linux- > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Neeli, Srinivas > <srinivas.neeli@amd.com> > Subject: Re: RE: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak > > Le 14/03/2023 à 06:15, Katakam, Harini a écrit : > > Hi Jakub, Jonas, > > > >> -----Original Message----- > >> From: Jakub Kicinski <kuba@kernel.org> > >> Sent: Tuesday, March 14, 2023 12:19 AM > >> To: Jonas Suhr Christensen <jsc@umbraculum.org>; Katakam, Harini > >> <harini.katakam@amd.com> > >> Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. > >> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > >> Michal Simek <michal.simek@xilinx.com>; Haoyue Xu > >> <xuhaoyue1@hisilicon.com>; huangjunxian > >> <huangjunxian6@hisilicon.com>; Wang Qing <wangqing@vivo.com>; Yang > >> Yingliang <yangyingliang@huawei.com>; Esben Haabendal > >> <esben@geanix.com>; linux-arm-kernel@lists.infradead.org; > >> linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak > >> > >> On Mon, 13 Mar 2023 19:37:00 +0100 Jonas Suhr Christensen wrote: > >>> On Tue, Feb 7, 2023, at 19:42, Jakub Kicinski wrote: > >>>> On Tue, 07 Feb 2023 12:36:11 +0100 Paolo Abeni wrote: > >>>>> You can either try change to phys type to __be32 (likely not > >>>>> suitable for -net and possibly can introduce even more warnings > >>>>> elsewhere) > >>>> > >>>> FWIW that seems like the best option to me as well. Let's ignore > >>>> the sparse warning for v3 and try to switch phys to __be32 in a > >>>> separate patch for net-next. No point adding force casts just to > >>>> have to remove them a week later, given how prevalent the problem is. > >>>> > >>>>> or explicitly cast the argument. > >>> > >>> I no longer have access to the hardware, so I'm not rewriting the > >>> batch. Feel free to take ownership of it and fix what's needed. > >> > >> Ack. > >> > >> Harini, are you the designated maintainer for this driver? Could you > >> add a MAINTAINERS entry for it? I don't see one right now. > >> And possibly pick up these patches / fix the problem, if you have the cycles? > > > > Sure, Srinivas (cced) will pick up this series and send a v3. > > I'll get back on the state of this IP/driver for the maintainers list. > > Will include that patch in the beginning of the series as well. > > > > Regards, > > Harini > > > > Hi, > > this patch, or an updated version, has not reached -next yet. > > Does someone still working on it, or did it got lost? Apologies, we dint get a chance to close on this. We'll fix and re-spin next month. This IP and driver are in minimal support mode right now. We'll update the maintainers entry. Regards, Harini
Hi, > -----Original Message----- > From: Katakam, Harini <harini.katakam@amd.com> > Sent: Monday, July 10, 2023 11:21 AM > To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Jakub Kicinski > <kuba@kernel.org>; Jonas Suhr Christensen <jsc@umbraculum.org> > Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > Michal Simek <michal.simek@xilinx.com>; Haoyue Xu > <xuhaoyue1@hisilicon.com>; huangjunxian <huangjunxian6@hisilicon.com>; > Wang Qing <wangqing@vivo.com>; Yang Yingliang > <yangyingliang@huawei.com>; Esben Haabendal <esben@geanix.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Neeli, > Srinivas <srinivas.neeli@amd.com>; Pandey, Radhey Shyam > <radhey.shyam.pandey@amd.com> > Subject: RE: RE: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak > > Hi Christophe, > > > -----Original Message----- > > From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > Sent: Saturday, July 8, 2023 6:46 PM > > To: Katakam, Harini <harini.katakam@amd.com>; Jakub Kicinski > > <kuba@kernel.org>; Jonas Suhr Christensen <jsc@umbraculum.org> > > Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; David S. > > Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > > Michal Simek <michal.simek@xilinx.com>; Haoyue Xu > > <xuhaoyue1@hisilicon.com>; huangjunxian > <huangjunxian6@hisilicon.com>; > > Wang Qing <wangqing@vivo.com>; Yang Yingliang > > <yangyingliang@huawei.com>; Esben Haabendal <esben@geanix.com>; > linux- > > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Neeli, > > Srinivas <srinivas.neeli@amd.com> > > Subject: Re: RE: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources > > leak > > <snip> > > >> > > >> Harini, are you the designated maintainer for this driver? Could > > >> you add a MAINTAINERS entry for it? I don't see one right now. > > >> And possibly pick up these patches / fix the problem, if you have the > cycles? > > > > > > Sure, Srinivas (cced) will pick up this series and send a v3. > > > I'll get back on the state of this IP/driver for the maintainers list. > > > Will include that patch in the beginning of the series as well. > > > > > > Regards, > > > Harini > > > > > > > Hi, > > > > this patch, or an updated version, has not reached -next yet. > > > > Does someone still working on it, or did it got lost? > > Apologies, we dint get a chance to close on this. We'll fix and re-spin next > month. > This IP and driver are in minimal support mode right now. > We'll update the maintainers entry. We checked internally for hardware to test this and concluded that this IP is no longer supported and driver cannot be maintained. I have just sent a patch to add an obsolete MAINTAINERS entry for this driver. Apologies for not updating the state earlier. Regards, Harini
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c index 1066420d6a83..74423adbe50d 100644 --- a/drivers/net/ethernet/xilinx/ll_temac_main.c +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c @@ -299,6 +299,7 @@ static int temac_dcr_setup(struct temac_local *lp, struct platform_device *op, static void temac_dma_bd_release(struct net_device *ndev) { struct temac_local *lp = netdev_priv(ndev); + struct cdmac_bd *bd; int i; /* Reset Local Link (DMA) */ @@ -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[i]; + 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. Fixes: fdd7454ecb29 ("net: ll_temac: Fix support for little-endian platforms") 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(-)