[PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[Yeounsu Moon]

Replace `dev_kfree_skb()` with `dev_kfree_skb_any()` in `start_xmit()`
which can be called from hard irq context (netpoll) and from other
contexts.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
Tested-on: D-Link DGE-550T Rev-A3
---
 drivers/net/ethernet/dlink/dl2k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index 1996d2e4e3e2..fcb89f9e5e2e 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -724,7 +724,7 @@ start_xmit (struct sk_buff *skb, struct net_device *dev)
 	u64 tfc_vlan_tag = 0;
 
 	if (np->link_status == 0) {	/* Link Down */
-		dev_kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return NETDEV_TX_OK;
 	}
 	entry = np->cur_tx % TX_RING_SIZE;
-- 
2.51.0

Re: [PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[Simon Horman]

On Fri, Oct 03, 2025 at 11:23:00AM +0900, Yeounsu Moon wrote:
> Replace `dev_kfree_skb()` with `dev_kfree_skb_any()` in `start_xmit()`
> which can be called from hard irq context (netpoll) and from other
> contexts.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
> Tested-on: D-Link DGE-550T Rev-A3

Hi,

I am curious to know why this problem has come up now.
Or more to the point, why it has not come up since the cited commit
was made, 20 years ago.

I am also curious to know how the problem was found.
By inspection? Through testing? Other?

...

Re: [PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[Yeounsu Moon]

On Fri Oct 3, 2025 at 5:17 PM KST, Simon Horman wrote:
> On Fri, Oct 03, 2025 at 11:23:00AM +0900, Yeounsu Moon wrote:
>> Replace `dev_kfree_skb()` with `dev_kfree_skb_any()` in `start_xmit()`
>> which can be called from hard irq context (netpoll) and from other
>> contexts.
>> 
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
>> Tested-on: D-Link DGE-550T Rev-A3
>
> Hi,
Hello, Simon!
>
> I am curious to know why this problem has come up now.
This came up because I have the hardware and recently dug into the code.
Until then, it was not considered an issue, because nobody raised it as
such.
> Or more to the point, why it has not come up since the cited commit
> was made, 20 years ago.
I think there are two combined reasons why it has not surfaced for two
decades:
1. very few people actually had this device/driver in use.
2. The problem is difficult to reproduce: one must use `netpoll`, and at
the same time the `link_speed` must drop to zero.
>
> I am also curious to know how the problem was found.
> By inspection? Through testing? Other?
While looking at the `dl2k.c` code, I noticed that its logic calls
either `dev_kfree_skb()` or `dev_consume_skb_irq()` depending on
interrupt context. That logic gave me the sense that a similar issue
could exist elsewhere.
>
> ...
And read other driver codes and commit messages, check `networking/netdevices`
(.ndo_start_xmit), enable `netpoll` and set up  `netconsole`, read
`net/core/netpoll.c`, read comment in `include/linux/netdevice.h`,
add countless `printk()`s, build millions of times... and so on.

Re: [PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[Simon Horman]

On Fri, Oct 03, 2025 at 07:51:25PM +0900, Yeounsu Moon wrote:
> On Fri Oct 3, 2025 at 5:17 PM KST, Simon Horman wrote:
> > On Fri, Oct 03, 2025 at 11:23:00AM +0900, Yeounsu Moon wrote:
> >> Replace `dev_kfree_skb()` with `dev_kfree_skb_any()` in `start_xmit()`
> >> which can be called from hard irq context (netpoll) and from other
> >> contexts.
> >> 
> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
> >> Tested-on: D-Link DGE-550T Rev-A3
> >
> > Hi,
> Hello, Simon!
> >
> > I am curious to know why this problem has come up now.
> This came up because I have the hardware and recently dug into the code.
> Until then, it was not considered an issue, because nobody raised it as
> such.
> > Or more to the point, why it has not come up since the cited commit
> > was made, 20 years ago.
> I think there are two combined reasons why it has not surfaced for two
> decades:
> 1. very few people actually had this device/driver in use.
> 2. The problem is difficult to reproduce: one must use `netpoll`, and at
> the same time the `link_speed` must drop to zero.
> >
> > I am also curious to know how the problem was found.
> > By inspection? Through testing? Other?
> While looking at the `dl2k.c` code, I noticed that its logic calls
> either `dev_kfree_skb()` or `dev_consume_skb_irq()` depending on
> interrupt context. That logic gave me the sense that a similar issue
> could exist elsewhere.
> >
> > ...
> And read other driver codes and commit messages, check `networking/netdevices`
> (.ndo_start_xmit), enable `netpoll` and set up  `netconsole`, read
> `net/core/netpoll.c`, read comment in `include/linux/netdevice.h`,
> add countless `printk()`s, build millions of times... and so on.

Hi Yeounsu,

Thanks for your detailed response.

I do think it would be useful to add something like this to the commit
message:

  Found by inspection.

But in any case, this patch looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net: dlink: use dev_kfree_skb_any instead of dev_kfree_skb

[Yeounsu Moon]

On Sat Oct 4, 2025 at 6:54 PM KST, Simon Horman wrote:
>
> Hi Yeounsu,
>
> Thanks for your detailed response.
I also appreciate your review :)
>
> I do think it would be useful to add something like this to the commit
> message:
>
>   Found by inspection.
>
After re-reading both your previous reply and my commit message, I can
see how question could arise. And I realize that I should have provided
a more convincing commit message, but I failed to do so.
Next time, I will make sure to include not only what you suggested but
also a more detailed commit message overall.

	Yeounsu Moon