| Message ID | e3646459ea67f10135ab821f90f66d8b6e74456c.1743497376.git.luying1@xiaomi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | usbnet:fix NPE during rx_complete | expand |
On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > From: luying1 <luying1@xiaomi.com> > > Missing usbnet_going_away Check in Critical Path. > The usb_submit_urb function lacks a usbnet_going_away > validation, whereas __usbnet_queue_skb includes this check. > > This inconsistency creates a race condition where: > A URB request may succeed, but the corresponding SKB data > fails to be queued. > > Subsequent processes: > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > attempt to access skb->next, triggering a NULL pointer > dereference (Kernel Panic). > > Signed-off-by: luying1 <luying1@xiaomi.com> Please use your name, not an email alias. Also, what commit id does this fix? Should it be applied to stable kernels? thanks, greg k-h
On Tue, Apr 1, 2025 at 6:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > > From: luying1 <luying1@xiaomi.com> > > > > Missing usbnet_going_away Check in Critical Path. > > The usb_submit_urb function lacks a usbnet_going_away > > validation, whereas __usbnet_queue_skb includes this check. > > > > This inconsistency creates a race condition where: > > A URB request may succeed, but the corresponding SKB data > > fails to be queued. > > > > Subsequent processes: > > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > > attempt to access skb->next, triggering a NULL pointer > > dereference (Kernel Panic). > > > > Signed-off-by: luying1 <luying1@xiaomi.com> > > Please use your name, not an email alias. > OK, I have updated. please check the Patch v2 > Also, what commit id does this fix? Should it be applied to stable > kernels? The commit id is 04e906839a053f092ef53f4fb2d610983412b904 (usbnet: fix cyclical race on disconnect with work queue) Should it be applied to stable kernels? -- Yes > thanks, > > greg k-h
On Tue, Apr 01, 2025 at 08:48:01PM +0800, Ying Lu wrote: > On Tue, Apr 1, 2025 at 6:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > > > From: luying1 <luying1@xiaomi.com> > > > > > > Missing usbnet_going_away Check in Critical Path. > > > The usb_submit_urb function lacks a usbnet_going_away > > > validation, whereas __usbnet_queue_skb includes this check. > > > > > > This inconsistency creates a race condition where: > > > A URB request may succeed, but the corresponding SKB data > > > fails to be queued. > > > > > > Subsequent processes: > > > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > > > attempt to access skb->next, triggering a NULL pointer > > > dereference (Kernel Panic). > > > > > > Signed-off-by: luying1 <luying1@xiaomi.com> > > > > Please use your name, not an email alias. > > > OK, I have updated. please check the Patch v2 > > > Also, what commit id does this fix? Should it be applied to stable > > kernels? > The commit id is 04e906839a053f092ef53f4fb2d610983412b904 > (usbnet: fix cyclical race on disconnect with work queue) > Should it be applied to stable kernels? -- Yes Please mark the commit with that information, you seem to have not done so for the v2 version :(
On Tue, Apr 1, 2025 at 9:48 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 01, 2025 at 08:48:01PM +0800, Ying Lu wrote: > > On Tue, Apr 1, 2025 at 6:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > > > > From: luying1 <luying1@xiaomi.com> > > > > > > > > Missing usbnet_going_away Check in Critical Path. > > > > The usb_submit_urb function lacks a usbnet_going_away > > > > validation, whereas __usbnet_queue_skb includes this check. > > > > > > > > This inconsistency creates a race condition where: > > > > A URB request may succeed, but the corresponding SKB data > > > > fails to be queued. > > > > > > > > Subsequent processes: > > > > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > > > > attempt to access skb->next, triggering a NULL pointer > > > > dereference (Kernel Panic). > > > > > > > > Signed-off-by: luying1 <luying1@xiaomi.com> > > > > > > Please use your name, not an email alias. > > > > > OK, I have updated. please check the Patch v2 > > > > > Also, what commit id does this fix? Should it be applied to stable > > > kernels? > > The commit id is 04e906839a053f092ef53f4fb2d610983412b904 > > (usbnet: fix cyclical race on disconnect with work queue) > > Should it be applied to stable kernels? -- Yes > > Please mark the commit with that information, you seem to have not done > so for the v2 version :( Thank you for your response. Could you please confirm if I understand correctly: Should we include in our commit message which commit id we're fixing?
On Wed, Apr 02, 2025 at 08:12:06AM +0800, Ying Lu wrote: > On Tue, Apr 1, 2025 at 9:48 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Apr 01, 2025 at 08:48:01PM +0800, Ying Lu wrote: > > > On Tue, Apr 1, 2025 at 6:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > > > > > From: luying1 <luying1@xiaomi.com> > > > > > > > > > > Missing usbnet_going_away Check in Critical Path. > > > > > The usb_submit_urb function lacks a usbnet_going_away > > > > > validation, whereas __usbnet_queue_skb includes this check. > > > > > > > > > > This inconsistency creates a race condition where: > > > > > A URB request may succeed, but the corresponding SKB data > > > > > fails to be queued. > > > > > > > > > > Subsequent processes: > > > > > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > > > > > attempt to access skb->next, triggering a NULL pointer > > > > > dereference (Kernel Panic). > > > > > > > > > > Signed-off-by: luying1 <luying1@xiaomi.com> > > > > > > > > Please use your name, not an email alias. > > > > > > > OK, I have updated. please check the Patch v2 > > > > > > > Also, what commit id does this fix? Should it be applied to stable > > > > kernels? > > > The commit id is 04e906839a053f092ef53f4fb2d610983412b904 > > > (usbnet: fix cyclical race on disconnect with work queue) > > > Should it be applied to stable kernels? -- Yes > > > > Please mark the commit with that information, you seem to have not done > > so for the v2 version :( > Thank you for your response. Could you please confirm if I understand correctly: > Should we include in our commit message which commit id we're fixing? No, use the correct "Fixes:" tag format as described in the documentation. thanks, greg k-h
On Wed, Apr 2, 2025 at 3:12 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 02, 2025 at 08:12:06AM +0800, Ying Lu wrote: > > On Tue, Apr 1, 2025 at 9:48 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Apr 01, 2025 at 08:48:01PM +0800, Ying Lu wrote: > > > > On Tue, Apr 1, 2025 at 6:31 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Tue, Apr 01, 2025 at 06:18:01PM +0800, Ying Lu wrote: > > > > > > From: luying1 <luying1@xiaomi.com> > > > > > > > > > > > > Missing usbnet_going_away Check in Critical Path. > > > > > > The usb_submit_urb function lacks a usbnet_going_away > > > > > > validation, whereas __usbnet_queue_skb includes this check. > > > > > > > > > > > > This inconsistency creates a race condition where: > > > > > > A URB request may succeed, but the corresponding SKB data > > > > > > fails to be queued. > > > > > > > > > > > > Subsequent processes: > > > > > > (e.g., rx_complete → defer_bh → __skb_unlink(skb, list)) > > > > > > attempt to access skb->next, triggering a NULL pointer > > > > > > dereference (Kernel Panic). > > > > > > > > > > > > Signed-off-by: luying1 <luying1@xiaomi.com> > > > > > > > > > > Please use your name, not an email alias. > > > > > > > > > OK, I have updated. please check the Patch v2 > > > > > > > > > Also, what commit id does this fix? Should it be applied to stable > > > > > kernels? > > > > The commit id is 04e906839a053f092ef53f4fb2d610983412b904 > > > > (usbnet: fix cyclical race on disconnect with work queue) > > > > Should it be applied to stable kernels? -- Yes > > > > > > Please mark the commit with that information, you seem to have not done > > > so for the v2 version :( > > Thank you for your response. Could you please confirm if I understand correctly: > > Should we include in our commit message which commit id we're fixing? > > No, use the correct "Fixes:" tag format as described in the > documentation. > > thanks, > > greg k-h Oh, I see. Thank you very much for the reminder! I've already fixed it in the PATCH v3. Could you please take another look? Thanks, Ying Lu
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 44179f4e807f..5161bb5d824b 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -519,7 +519,8 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) netif_device_present (dev->net) && test_bit(EVENT_DEV_OPEN, &dev->flags) && !test_bit (EVENT_RX_HALT, &dev->flags) && - !test_bit (EVENT_DEV_ASLEEP, &dev->flags)) { + !test_bit (EVENT_DEV_ASLEEP, &dev->flags) && + !usbnet_going_away(dev)) { switch (retval = usb_submit_urb (urb, GFP_ATOMIC)) { case -EPIPE: usbnet_defer_kevent (dev, EVENT_RX_HALT); @@ -540,8 +541,7 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) tasklet_schedule (&dev->bh); break; case 0: - if (!usbnet_going_away(dev)) - __usbnet_queue_skb(&dev->rxq, skb, rx_start); + __usbnet_queue_skb(&dev->rxq, skb, rx_start); } } else { netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");