This patch series fixes an error in the ll_temac driver releated to
dma mapping.
The main change adds missing conversion of address when unampping
dma.
Changes in v2:
- Fix wrong (static) indexing when iterating in for-loop
- Variable declaration order longest to shortest
- Add fixes tags
- improve commit message
Jonas Suhr Christensen (2):
net: ll_temac: Fix DMA resources leak
net: ll_temac: Reset buffer on dma_map_single() errors
drivers/net/ethernet/xilinx/ll_temac_main.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
--
2.39.1
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 <[email protected]>
---
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..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,
--
2.39.1
To avoid later calls to dma_unmap_single() on address'
that fails to be mapped, free the allocated skb and
set the pointer of the address to NULL. Eg. when a mapping
fails temac_dma_bd_release() will try to call dma_unmap_single()
on that address if the structure is not reset.
Fixes: d07c849cd2b9 ("net: ll_temac: Add more error handling of dma_map_single() calls")
Signed-off-by: Jonas Suhr Christensen <[email protected]>
---
drivers/net/ethernet/xilinx/ll_temac_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
index 74423adbe50d..df43f5bc3bd3 100644
--- a/drivers/net/ethernet/xilinx/ll_temac_main.c
+++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
@@ -376,8 +376,11 @@ static int temac_dma_bd_init(struct net_device *ndev)
skb_dma_addr = dma_map_single(ndev->dev.parent, skb->data,
XTE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
- if (dma_mapping_error(ndev->dev.parent, skb_dma_addr))
+ if (dma_mapping_error(ndev->dev.parent, skb_dma_addr)) {
+ dev_kfree_skb(lp->rx_skb[i]);
+ lp->rx_skb[i] = NULL;
goto out;
+ }
lp->rx_bd_v[i].phys = cpu_to_be32(skb_dma_addr);
lp->rx_bd_v[i].len = cpu_to_be32(XTE_MAX_JUMBO_FRAME_SIZE);
lp->rx_bd_v[i].app0 = cpu_to_be32(STS_CTRL_APP0_IRQONEND);
--
2.39.1
Hi Jonas,
> -----Original Message-----
> From: Jonas Suhr Christensen <[email protected]>
> Sent: Monday, February 6, 2023 1:41 AM
> To: [email protected]
> Cc: [email protected]; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Michal Simek
> <[email protected]>; Katakam, Harini <[email protected]>;
> Haoyue Xu <[email protected]>; huangjunxian
> <[email protected]>; Christophe JAILLET
> <[email protected]>; Yang Yingliang
> <[email protected]>; Esben Haabendal <[email protected]>;
> [email protected]; [email protected]
> Subject: [PATCH net v2 2/2] net: ll_temac: Reset buffer on dma_map_single()
> errors
>
> To avoid later calls to dma_unmap_single() on address'
> that fails to be mapped, free the allocated skb and
> set the pointer of the address to NULL. Eg. when a mapping
> fails temac_dma_bd_release() will try to call dma_unmap_single()
> on that address if the structure is not reset.
>
> Fixes: d07c849cd2b9 ("net: ll_temac: Add more error handling of
> dma_map_single() calls")
>
> Signed-off-by: Jonas Suhr Christensen <[email protected]>
Thanks for the patch
Reviewed-by: Harini Katakam <[email protected]>
Regards,
Harini
> -----Original Message-----
> From: Jonas Suhr Christensen <[email protected]>
> Sent: Monday, February 6, 2023 1:41 AM
> To: [email protected]
> Cc: [email protected]; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Michal Simek
> <[email protected]>; Katakam, Harini <[email protected]>;
> Haoyue Xu <[email protected]>; huangjunxian
> <[email protected]>; Wang Qing <[email protected]>; Yang
> Yingliang <[email protected]>; Esben Haabendal
> <[email protected]>; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
Thanks for the patch.
Reviewed-by: Harini Katakam <[email protected]>
Regards,
Harini
Jonas Suhr Christensen <[email protected]> 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 <[email protected]>
Acked-by: Esben Haabendal <[email protected]>
Jonas Suhr Christensen <[email protected]> writes:
> To avoid later calls to dma_unmap_single() on address'
> that fails to be mapped, free the allocated skb and
> set the pointer of the address to NULL. Eg. when a mapping
> fails temac_dma_bd_release() will try to call dma_unmap_single()
> on that address if the structure is not reset.
>
> Fixes: d07c849cd2b9 ("net: ll_temac: Add more error handling of dma_map_single() calls")
>
> Signed-off-by: Jonas Suhr Christensen <[email protected]>
Acked-by: Esben Haabendal <[email protected]>
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 <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> Sent: Tuesday, March 14, 2023 12:19 AM
> To: Jonas Suhr Christensen <[email protected]>; Katakam, Harini
> <[email protected]>
> Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Michal Simek <[email protected]>; Haoyue Xu
> <[email protected]>; huangjunxian <[email protected]>;
> Wang Qing <[email protected]>; Yang Yingliang
> <[email protected]>; Esben Haabendal <[email protected]>;
> [email protected]; [email protected]
> 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 <[email protected]>
>> Sent: Tuesday, March 14, 2023 12:19 AM
>> To: Jonas Suhr Christensen <[email protected]>; Katakam, Harini
>> <[email protected]>
>> Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
>> Miller <[email protected]>; Eric Dumazet <[email protected]>;
>> Michal Simek <[email protected]>; Haoyue Xu
>> <[email protected]>; huangjunxian <[email protected]>;
>> Wang Qing <[email protected]>; Yang Yingliang
>> <[email protected]>; Esben Haabendal <[email protected]>;
>> [email protected]; [email protected]
>> 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 <[email protected]>
> Sent: Saturday, July 8, 2023 6:46 PM
> To: Katakam, Harini <[email protected]>; Jakub Kicinski
> <[email protected]>; Jonas Suhr Christensen <[email protected]>
> Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Michal Simek <[email protected]>; Haoyue Xu
> <[email protected]>; huangjunxian <[email protected]>;
> Wang Qing <[email protected]>; Yang Yingliang
> <[email protected]>; Esben Haabendal <[email protected]>; linux-
> [email protected]; [email protected]; Neeli, Srinivas
> <[email protected]>
> 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 <[email protected]>
> >> Sent: Tuesday, March 14, 2023 12:19 AM
> >> To: Jonas Suhr Christensen <[email protected]>; Katakam, Harini
> >> <[email protected]>
> >> Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
> >> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> >> Michal Simek <[email protected]>; Haoyue Xu
> >> <[email protected]>; huangjunxian
> >> <[email protected]>; Wang Qing <[email protected]>; Yang
> >> Yingliang <[email protected]>; Esben Haabendal
> >> <[email protected]>; [email protected];
> >> [email protected]
> >> 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 <[email protected]>
> Sent: Monday, July 10, 2023 11:21 AM
> To: Christophe JAILLET <[email protected]>; Jakub Kicinski
> <[email protected]>; Jonas Suhr Christensen <[email protected]>
> Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Michal Simek <[email protected]>; Haoyue Xu
> <[email protected]>; huangjunxian <[email protected]>;
> Wang Qing <[email protected]>; Yang Yingliang
> <[email protected]>; Esben Haabendal <[email protected]>;
> [email protected]; [email protected]; Neeli,
> Srinivas <[email protected]>; Pandey, Radhey Shyam
> <[email protected]>
> Subject: RE: RE: [PATCH net v2 1/2] net: ll_temac: Fix DMA resources leak
>
> Hi Christophe,
>
> > -----Original Message-----
> > From: Christophe JAILLET <[email protected]>
> > Sent: Saturday, July 8, 2023 6:46 PM
> > To: Katakam, Harini <[email protected]>; Jakub Kicinski
> > <[email protected]>; Jonas Suhr Christensen <[email protected]>
> > Cc: Paolo Abeni <[email protected]>; [email protected]; David S.
> > Miller <[email protected]>; Eric Dumazet <[email protected]>;
> > Michal Simek <[email protected]>; Haoyue Xu
> > <[email protected]>; huangjunxian
> <[email protected]>;
> > Wang Qing <[email protected]>; Yang Yingliang
> > <[email protected]>; Esben Haabendal <[email protected]>;
> linux-
> > [email protected]; [email protected]; Neeli,
> > Srinivas <[email protected]>
> > 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