* Re: [PATCH] net: hso: fix tx skb leak when the device stops
2026-11-14 9:31 [PATCH] net: hso: fix tx skb leak when the device stops Aibing Chen
@ 2026-11-15 9:31 ` sashiko-bot
2026-11-15 10:16 ` [PATCH v2] net: hso: fix tx skb leak and a race on the stashed skb Aibing Chen
2026-11-15 21:20 ` [PATCH] net: hso: fix tx skb leak when the device stops netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-11-15 9:31 UTC (permalink / raw)
To: Aibing Chen; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: hso: race on skb_tx_buf between hso_net_close() and
hso_resume()
- [Low] net: hso: missing Fixes trailer in commit message
--
commit 759e4b053ce61e8448ad5b49402e8bd576be78fd
Author: Aibing Chen <chenaibing@cmss.chinamobile.com>
> net: hso: fix tx skb leak when the device stops
>
> hso_net_start_xmit() stashes the skb in odev->skb_tx_buf and stops the
> queue when hso_get_activity() reports -EAGAIN, which happens when the
> USB device is suspended or no longer configured. If the interface is
> then brought down, or the device is disconnected, that skb is simply
> left behind: hso_net_close() and hso_free_net_device() never release
> it, and hso_net_open() clears the pointer, so the skb and its data
> buffer are leaked.
>
> Free the pending skb in hso_net_close(), which is also reached from
> unregister_netdev() on disconnect.
>
> hso_resume() loses a frame in the same way when the device is still not
> ready: it passes skb_tx_buf to hso_net_start_xmit() and then clears the
> pointer unconditionally, so a frame stashed again by that call is never
> freed. Clear the pointer before the call instead and let
> hso_net_start_xmit() stash it again if it has to.
>
> Reported-by: syzbot+bf62ca9997f43d2fa521@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
> Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>
[Severity: Low]
Should this commit include Fixes tags for the two commits that introduced
these skb_tx_buf leaks?
Fixes: 72dc1c096c70 ("HSO: add option hso driver")
Fixes: c213f286f2cf ("[netdrvr] hso: dev_kfree_skb crash fix")
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 71caa3764b239..03956fa8ee537 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -728,6 +728,13 @@ static int hso_net_close(struct net_device *net)
>
> hso_stop_net_device(odev->parent);
>
> + /*
> + * A frame stashed by hso_net_start_xmit() will never be transmitted now,
> + * so drop it instead of leaking it.
> + */
> + dev_kfree_skb(odev->skb_tx_buf);
> + odev->skb_tx_buf = NULL;
> +
> /* done */
> return 0;
> }
> @@ -3055,12 +3062,13 @@ static int hso_resume(struct usb_interface *iface)
> /* First transmit any lingering data,
> then restart the device. */
> if (hso_net->skb_tx_buf) {
> + struct sk_buff *skb = hso_net->skb_tx_buf;
> +
> dev_dbg(&iface->dev,
> "Transmitting"
> " lingering data\n");
> - hso_net_start_xmit(hso_net->skb_tx_buf,
> - hso_net->net);
> hso_net->skb_tx_buf = NULL;
> + hso_net_start_xmit(skb, hso_net->net);
> }
> result = hso_start_net_device(network_table[i]);
> if (result)
[Severity: High]
Can hso_net_close() and hso_resume() race on skb_tx_buf and cause a
use-after-free and double free?
When hso_net_start_xmit() stashes a frame while the USB device is
suspended, hso_get_activity() schedules async_get_intf to resume the
interface. hso_net_close() runs under rtnl_lock without holding the USB
device lock, cancelling async_get_intf, or synchronizing with hso_resume():
CPU0 CPU1
hso_resume() hso_net_close()
if (hso_net->flags & IFF_UP)
skb = hso_net->skb_tx_buf;
clear_bit(HSO_NET_RUNNING, &odev->flags);
hso_stop_net_device(odev->parent);
dev_kfree_skb(odev->skb_tx_buf);
odev->skb_tx_buf = NULL;
hso_net->skb_tx_buf = NULL;
hso_net_start_xmit(skb, ...)
memcpy(..., skb->data, ...) <-- use-after-free
dev_kfree_skb(skb) <-- double free
If CPU0 clears hso_net->skb_tx_buf just before CPU1 calls dev_kfree_skb(),
CPU1 frees NULL and finishes closing the device, after which CPU0 calls
hso_net_start_xmit() and hso_start_net_device() on the stopped device,
either submitting URBs after close or re-stashing and leaking skb in
skb_tx_buf.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261114093133.465-1-chenaibing@cmss.chinamobile.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2] net: hso: fix tx skb leak and a race on the stashed skb
2026-11-14 9:31 [PATCH] net: hso: fix tx skb leak when the device stops Aibing Chen
2026-11-15 9:31 ` sashiko-bot
@ 2026-11-15 10:16 ` Aibing Chen
2026-11-15 21:20 ` [PATCH] net: hso: fix tx skb leak when the device stops netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: Aibing Chen @ 2026-11-15 10:16 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: linux-usb, netdev, linux-kernel
hso_net_start_xmit() stashes the skb in odev->skb_tx_buf and stops the
queue when hso_get_activity() reports -EAGAIN, which happens when the
USB device is suspended or no longer configured. If the interface is
then brought down, or the device is disconnected, that skb is simply
left behind: hso_net_close() and hso_free_net_device() never release
it, and hso_net_open() clears the pointer, so the skb and its data
buffer are leaked.
hso_resume() loses a frame in the same way when the device is still not
ready: it passes skb_tx_buf to hso_net_start_xmit() and then clears the
pointer unconditionally, so a frame stashed again by that call is never
freed.
Free the pending skb in hso_net_close() and take ownership of the
pointer with xchg() in both places, so that only one of the close and
resume paths can obtain it. hso_resume() also drops the frame instead
of transmitting it when the interface has already been stopped.
Reported-by: syzbot+bf62ca9997f43d2fa521@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
Fixes: 72dc1c096c70 ("HSO: add option hso driver")
Fixes: c213f286f2cf ("[netdrvr] hso: dev_kfree_skb crash fix")
Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>
---
v2:
- take the stashed skb with xchg() in hso_net_close() and hso_resume(),
so that only one path can free it (race reported by sashiko).
- drop the frame in hso_resume() instead of transmitting it after the
interface was stopped.
- add the two Fixes tags.
drivers/net/usb/hso.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 71caa3764..ba59bdefd 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -720,6 +720,7 @@ static int hso_net_open(struct net_device *net)
static int hso_net_close(struct net_device *net)
{
struct hso_net *odev = netdev_priv(net);
+ struct sk_buff *skb;
/* we don't need the queue anymore */
netif_stop_queue(net);
@@ -728,6 +729,14 @@ static int hso_net_close(struct net_device *net)
hso_stop_net_device(odev->parent);
+ /*
+ * Take ownership of a frame stashed by hso_net_start_xmit() and drop it.
+ * hso_resume() takes it the same way, so only one of the two paths frees
+ * it.
+ */
+ skb = xchg(&odev->skb_tx_buf, NULL);
+ dev_kfree_skb(skb);
+
/* done */
return 0;
}
@@ -3032,6 +3041,7 @@ static int hso_resume(struct usb_interface *iface)
{
int i, result = 0;
struct hso_net *hso_net;
+ struct sk_buff *skb;
/* Start all serial ports */
for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++) {
@@ -3054,13 +3064,17 @@ static int hso_resume(struct usb_interface *iface)
if (hso_net->flags & IFF_UP) {
/* First transmit any lingering data,
then restart the device. */
- if (hso_net->skb_tx_buf) {
+ skb = xchg(&hso_net->skb_tx_buf, NULL);
+ if (skb) {
dev_dbg(&iface->dev,
"Transmitting"
" lingering data\n");
- hso_net_start_xmit(hso_net->skb_tx_buf,
- hso_net->net);
- hso_net->skb_tx_buf = NULL;
+ if (test_bit(HSO_NET_RUNNING,
+ &hso_net->flags))
+ hso_net_start_xmit(skb,
+ hso_net->net);
+ else
+ dev_kfree_skb(skb);
}
result = hso_start_net_device(network_table[i]);
if (result)
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] net: hso: fix tx skb leak when the device stops
2026-11-14 9:31 [PATCH] net: hso: fix tx skb leak when the device stops Aibing Chen
2026-11-15 9:31 ` sashiko-bot
2026-11-15 10:16 ` [PATCH v2] net: hso: fix tx skb leak and a race on the stashed skb Aibing Chen
@ 2026-11-15 21:20 ` netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-11-15 21:20 UTC (permalink / raw)
To: chenaibing
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, linux-usb, netdev,
linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 0 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The patch adds an unsynchronized free of odev->skb_tx_buf in
hso_net_close() (drivers/net/usb/hso.c:735-736).
- [Low] The newly added free in hso_net_close()
(drivers/net/usb/hso.c:731-736) discards a frame that was accepted…
- [Low] The patch fixes a user-visible, syzbot-reported skb leak but
carries no Fixes: tag (and no Cc: stable).
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
hso_net_start_xmit() (drivers/net/usb/hso.c:789) does…
--
net: hso: fix tx skb leak when the device stops
This patch frees the skb that hso_net_start_xmit() stashes in
skb_tx_buf when it runs in hso_net_close(), which is also reached on
disconnect. It also changes hso_resume() so that skb_tx_buf is cleared
before the lingering frame is passed back to hso_net_start_xmit().
Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
> Reported-by: syzbot+bf62ca9997f43d2fa521@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/6af74e31.fe96acdb.12252b.0009.GAE@google.com/
> Signed-off-by: Aibing Chen <chenaibing@cmss.chinamobile.com>
[Severity: Low]
This patch fixes a leak that syzbot reported, but it has no Fixes: tag.
The resume path ordering changed here seems to come from commit
89930b7b5e3e ("hso: Fix URB submission -EINVAL."). The stash and
teardown logic dates back to commit 72dc1c096c70 ("HSO: add option hso
driver").
Should one of these be added as a Fixes: tag, along with Cc: stable?
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 71caa3764b239..03956fa8ee537 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -728,6 +728,13 @@ static int hso_net_close(struct net_device *net)
>
> hso_stop_net_device(odev->parent);
>
> + /*
> + * A frame stashed by hso_net_start_xmit() will never be transmitted now,
> + * so drop it instead of leaking it.
> + */
> + dev_kfree_skb(odev->skb_tx_buf);
[Severity: Low]
The comment says the frame is dropped, but dev_kfree_skb() is defined as
consume_skb(). The kfree_skb drop tracepoint and drop monitor will
never see this frame. net->stats.tx_dropped is not incremented either.
Would kfree_skb() or kfree_skb_reason(), plus net->stats.tx_dropped++,
match the intent better?
> + odev->skb_tx_buf = NULL;
> +
> /* done */
> return 0;
> }
> @@ -3055,12 +3062,13 @@ static int hso_resume(struct usb_interface *iface)
> /* First transmit any lingering data,
> then restart the device. */
> if (hso_net->skb_tx_buf) {
> + struct sk_buff *skb = hso_net->skb_tx_buf;
> +
> dev_dbg(&iface->dev,
> "Transmitting"
> " lingering data\n");
> - hso_net_start_xmit(hso_net->skb_tx_buf,
> - hso_net->net);
> hso_net->skb_tx_buf = NULL;
> + hso_net_start_xmit(skb, hso_net->net);
[Severity: High]
Can the new free in hso_net_close() race with this replay of the
lingering frame?
hso_net_close() runs as ndo_stop and holds only RTNL. hso_resume() runs
as the USB resume callback. One way it gets there is the async_get_intf
work, which hso_get_activity() schedules at the same moment the frame
is stashed:
hso_get_activity() {
...
if (hso_dev->usb->state == USB_STATE_SUSPENDED) {
if (!hso_dev->is_active) {
hso_dev->is_active = 1;
schedule_work(&hso_dev->async_get_intf);
...
}
The two paths share no lock, so this interleaving looks possible:
CPU1 (hso_resume) CPU2 (hso_net_close)
if (hso_net->flags & IFF_UP)
if (hso_net->skb_tx_buf)
skb = hso_net->skb_tx_buf;
clear_bit(HSO_NET_RUNNING, ...);
hso_stop_net_device(...);
dev_kfree_skb(odev->skb_tx_buf);
hso_net->skb_tx_buf = NULL;
hso_net_start_xmit(skb, ...)
memcpy(..., skb->data, skb->len);
...
dev_kfree_skb(skb);
The device is configured again during resume. hso_net_start_xmit()
would therefore read the freed skb data, send it to the device, and then
free the skb a second time.
Before this patch, hso_net_close() never freed skb_tx_buf, so this race
could only cause a late transmit or a leak.
Clearing the pointer earlier in hso_resume() does not seem to close the
window. The load and the clear are still not atomic with respect to
close.
Could the stash, the replay and the free be serialized, for example
with net_lock or netif_tx_lock? HSO_NET_RUNNING could then be rechecked
under that lock. Another option would be
cancel_work_sync(&async_get_intf) in hso_net_close().
[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Since
the patch adds another caller of hso_net_start_xmit(), though: can that
function overflow mux_bulk_tx_buf?
hso_net_start_xmit() copies skb->len bytes without checking them
against MUX_BULK_TX_BUF_SIZE:
memcpy(odev->mux_bulk_tx_buf, skb->data, skb->len);
The buffer has a fixed size, set in hso_create_net_device():
hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
hso_net_init() only sets net->mtu = DEFAULT_MTU - 14 and never sets
net->max_mtu. As a result, dev_validate_mtu() skips the upper bound
check:
if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {
After something like "ip link set hsoX mtu 9000", any packet larger
than 2048 bytes would overflow the kmalloc-2048 object. skb->len is
also used as the URB transfer length, so the USB controller would read
past the buffer as well.
Should hso_net_init() set net->max_mtu, or should
hso_net_start_xmit() check skb->len before the memcpy()?
> }
> result = hso_start_net_device(network_table[i]);
> if (result)
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114093133.465-1-chenaibing%40cmss.chinamobile.com
^ permalink raw reply [flat|nested] 4+ messages in thread