Re: [PATCH net v4] usbnet: fix smp_processor_id() use in preemptible context
From: netdev-bot+sashiko
Date: Wed Sep 09 2026 - 17:55:34 EST
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 0
- [High] usbnet: dropping
u64_stats_update_begin_irqsave()/u64_stats_update_end_irqrestore() in…
--
usbnet: fix smp_processor_id() use in preemptible context
This changes usbnet_skb_return() and tx_complete() to fetch the per-CPU
pcpu_sw_netstats pointer with get_cpu_ptr()/put_cpu_ptr() instead of a bare
this_cpu_ptr(), addressing a smp_processor_id()-in-preemptible-code report
from syzbot. Along with the pointer change it also switches the seqcount
write helpers from the _irqsave variants to the plain ones.
Closes: https://syzkaller.appspot.com/bug?extid=04cd90bb99c6ef81a65d
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index a19ecf718f36a..3df72b0c6bcfc 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -325,8 +325,7 @@ static void __usbnet_status_stop_force(struct usbnet *dev)
> */
> void usbnet_skb_return(struct usbnet *dev, struct sk_buff *skb)
> {
> - struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->net->tstats);
> - unsigned long flags;
> + struct pcpu_sw_netstats *stats64;
> int status;
>
> if (test_bit(EVENT_RX_PAUSED, &dev->flags)) {
> @@ -338,10 +337,12 @@ void usbnet_skb_return(struct usbnet *dev, struct sk_buff *skb)
> if (skb->protocol == 0)
> skb->protocol = eth_type_trans(skb, dev->net);
>
> - flags = u64_stats_update_begin_irqsave(&stats64->syncp);
> + stats64 = get_cpu_ptr(dev->net->tstats);
> + u64_stats_update_begin(&stats64->syncp);
> u64_stats_inc(&stats64->rx_packets);
> u64_stats_add(&stats64->rx_bytes, skb->len);
> - u64_stats_update_end_irqrestore(&stats64->syncp, flags);
> + u64_stats_update_end(&stats64->syncp);
> + put_cpu_ptr(dev->net->tstats);
[Severity: High]
Does dropping the _irqsave seqcount helpers here re-open the problem that
commit 2695578b896a ("net: usbnet: fix potential deadlock on 32bit hosts")
closed?
The reported splat is about the per-CPU pointer being taken outside of a
preempt-disabled section, which get_cpu_ptr() addresses. But get_cpu_ptr()
only disables preemption:
include/linux/percpu-defs.h
#define get_cpu_ptr(var) \
({ \
preempt_disable(); \
this_cpu_ptr(var); \
})
so nothing masks interrupts across the seqcount write section anymore.
usbnet_skb_return() runs from usbnet_bh_work()->rx_process() (and from
usbnet_resume_rx() in BH-disabled process context), i.e. with hardirqs
enabled. tx_complete() below is the urb->complete callback, and for host
controllers that do not set HCD_BH it is invoked straight from the
controller's hardirq handler:
drivers/usb/core/hcd.c:usb_hcd_giveback_urb() {
if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
__usb_hcd_giveback_urb(urb);
return;
}
}
drivers/usb/core/hcd.c:__usb_hcd_giveback_urb() {
kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum);
urb->complete(urb);
kcov_remote_stop_softirq();
}
There is no local_irq_save() around urb->complete(). In-tree only
ehci-hcd, xhci and dwc2 set HCD_BH, so on OHCI/UHCI/musb/max3421/sl811 and
friends tx_complete() is a hardirq-context writer.
Both sites write the same object, since struct pcpu_sw_netstats has a
single u64_stats_sync shared by the rx and tx counters:
include/linux/netdevice.h
struct pcpu_sw_netstats {
u64_stats_t rx_packets;
u64_stats_t rx_bytes;
u64_stats_t tx_packets;
u64_stats_t tx_bytes;
struct u64_stats_sync syncp;
};
On 32-bit, where u64_stats_sync is a real seqcount, can the following
nesting happen on one CPU?
usbnet_bh_work() -> rx_process() -> usbnet_skb_return()
u64_stats_update_begin() /* seq odd, rx_bytes half written */
<HCD hardirq>
usb_hcd_giveback_urb() -> __usb_hcd_giveback_urb() -> tx_complete()
u64_stats_update_begin() /* seq even again */
u64_stats_update_end() /* seq odd */
u64_stats_update_end() /* seq even */
A reader coming in via dev_get_tstats64()->dev_fetch_sw_netstats() (for
example "ip -s link show") would then see an even sequence in the middle of
the outer update and accept a torn 64-bit counter without
u64_stats_fetch_retry() firing. With lockdep on 32-bit, is this also the
"inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W}" report that 2695578b896a
removed?
This is the constraint documented in include/linux/u64_stats_sync.h:
* 3) Write side must use the _irqsave() variant if other writers, or a reader,
* can be invoked from an IRQ context. On 64bit systems this variant does not
* disable interrupts.
Since the _irqsave variant compiles away on 64-bit, would it work to keep
it and only change the pointer acquisition?
stats64 = get_cpu_ptr(dev->net->tstats);
flags = u64_stats_update_begin_irqsave(&stats64->syncp);
u64_stats_inc(&stats64->rx_packets);
u64_stats_add(&stats64->rx_bytes, skb->len);
u64_stats_update_end_irqrestore(&stats64->syncp, flags);
put_cpu_ptr(dev->net->tstats);
The changelog only mentions the get_cpu_ptr()/put_cpu_ptr() conversion and
says nothing about dropping the IRQ-safe helpers; could that be spelled out
if the removal is intentional?
>
> netif_dbg(dev, rx_status, dev->net, "< rx, len %zu, type 0x%x\n",
> skb->len + sizeof(struct ethhdr), skb->protocol);
> @@ -1298,13 +1299,14 @@ static void tx_complete(struct urb *urb)
> struct usbnet *dev = entry->dev;
>
> if (urb->status == 0) {
> - struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->net->tstats);
> - unsigned long flags;
> + struct pcpu_sw_netstats *stats64;
>
> - flags = u64_stats_update_begin_irqsave(&stats64->syncp);
> + stats64 = get_cpu_ptr(dev->net->tstats);
> + u64_stats_update_begin(&stats64->syncp);
> u64_stats_add(&stats64->tx_packets, entry->packets);
> u64_stats_add(&stats64->tx_bytes, entry->length);
> - u64_stats_update_end_irqrestore(&stats64->syncp, flags);
> + u64_stats_update_end(&stats64->syncp);
> + put_cpu_ptr(dev->net->tstats);
> } else {
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907214727.692307-1-omermetekaya0%40gmail.com