| Message ID | 20241231161539.20192-2-xndchn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usbip: Fix seqnum sign extension and format specifier issues | expand |
On 12/31/24 09:15, Xiong Nandi wrote: > The atomic_inc_return function returns an int, while priv->seqnum is an > unsigned long. So we must cast the result to u32 to prevent potential > sign extension and size mismatch issues. > How did you find the problem? > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > --- > drivers/usb/usbip/vhci_hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > index b03e5021c25b..f3f260e01791 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > spin_lock_irqsave(&vdev->priv_lock, flags); > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); Why does it make sense to cast it to u32? > if (priv->seqnum == 0xffff) > dev_info(&urb->dev->dev, "seqnum max\n"); thanks, -- Shuah
Thanks. > How did you find the problem? > Why does it make sense to cast it to u32? After running with usbip enough time, I happened to see logs like this: > [ 293.863125] vhci_hcd vhci_hcd.0: Device attached > [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 > [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > [ 294.204850] vhci_hcd: stop threads > [ 294.204851] vhci_hcd: release socket > [ 294.204853] vhci_hcd: disconnect device Then I notice that on 64bit platform, when atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or 0x80000000), priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be extends to 0xffffffff80000000 So we can fix the issue by cast it to u32. On Fri, Jan 3, 2025 at 6:06 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 12/31/24 09:15, Xiong Nandi wrote: > > The atomic_inc_return function returns an int, while priv->seqnum is an > > unsigned long. So we must cast the result to u32 to prevent potential > > sign extension and size mismatch issues. > > > > How did you find the problem? > > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > > --- > > drivers/usb/usbip/vhci_hcd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > > index b03e5021c25b..f3f260e01791 100644 > > --- a/drivers/usb/usbip/vhci_hcd.c > > +++ b/drivers/usb/usbip/vhci_hcd.c > > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > > > spin_lock_irqsave(&vdev->priv_lock, flags); > > > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); > > Why does it make sense to cast it to u32? > > > if (priv->seqnum == 0xffff) > > dev_info(&urb->dev->dev, "seqnum max\n"); > > > thanks, > -- Shuah
On 1/3/25 08:18, xndcn wrote: > Thanks. > >> How did you find the problem? >> Why does it make sense to cast it to u32? > > After running with usbip enough time, I happened to see logs like this: >> [ 293.863125] vhci_hcd vhci_hcd.0: Device attached >> [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd >> [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 >> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 >> [ 294.204850] vhci_hcd: stop threads >> [ 294.204851] vhci_hcd: release socket >> [ 294.204853] vhci_hcd: disconnect device > > Then I notice that on 64bit platform, when > atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or > 0x80000000), > priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be > extends to 0xffffffff80000000 > So we can fix the issue by cast it to u32. > Can you send me the dmesg without and with your patch? thanks, -- Shuah
without the patch: > [ 384.276605] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 384.278487] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 1 > [ 384.278509] vhci_hcd: created sysfs vhci_hcd.0 > [ 384.278532] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.12 > [ 384.278535] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 384.278536] usb usb1: Product: USB/IP Virtual Host Controller > [ 384.278538] usb usb1: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 384.278539] usb usb1: SerialNumber: vhci_hcd.0 > [ 384.278630] hub 1-0:1.0: USB hub found > [ 384.278637] hub 1-0:1.0: 8 ports detected > [ 384.278740] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 384.278781] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 2 > [ 384.278788] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > [ 384.278801] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, bcdDevice= 6.12 > [ 384.278802] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 384.278803] usb usb2: Product: USB/IP Virtual Host Controller > [ 384.278804] usb usb2: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 384.278805] usb usb2: SerialNumber: vhci_hcd.0 > [ 384.278866] hub 2-0:1.0: USB hub found > [ 384.278869] hub 2-0:1.0: 8 ports detected > [ 384.279071] insmod (400) used greatest stack depth: 11960 bytes left > [ 550.127351] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3) > [ 550.127356] vhci_hcd vhci_hcd.0: devid(0) speed(3) speed_str(high-speed) > [ 550.127359] vhci_hcd vhci_hcd.0: Device attached > [ 550.341007] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 550.452995] usb 1-1: SetAddress Request (2) to port 0 > [ 550.464332] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > [ 550.464842] vhci_hcd: stop threads > [ 550.464843] vhci_hcd: release socket > [ 550.464845] vhci_hcd: disconnect device and with this patch: > [ 746.253823] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 746.254660] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 1 > [ 746.254669] vhci_hcd: created sysfs vhci_hcd.0 > [ 746.254895] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.12 > [ 746.254897] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 746.254898] usb usb1: Product: USB/IP Virtual Host Controller > [ 746.254899] usb usb1: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 746.254899] usb usb1: SerialNumber: vhci_hcd.0 > [ 746.254964] hub 1-0:1.0: USB hub found > [ 746.254985] hub 1-0:1.0: 8 ports detected > [ 746.255042] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 746.255066] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 2 > [ 746.255072] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > [ 746.255081] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, bcdDevice= 6.12 > [ 746.255082] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 746.255083] usb usb2: Product: USB/IP Virtual Host Controller > [ 746.255089] usb usb2: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 746.255089] usb usb2: SerialNumber: vhci_hcd.0 > [ 746.255118] hub 2-0:1.0: USB hub found > [ 746.255120] hub 2-0:1.0: 8 ports detected > [ 756.967922] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3) > [ 756.967928] vhci_hcd vhci_hcd.0: devid(0) speed(3) speed_str(high-speed) > [ 756.967933] vhci_hcd vhci_hcd.0: Device attached > [ 757.184367] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 757.296479] usb 1-1: SetAddress Request (2) to port 0 > [ 757.309845] usb 1-1: New USB device found, idVendor=1234, idProduct=1234, bcdDevice= 2.80 > [ 757.309848] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > [ 757.309849] usb 1-1: Product: foo > [ 757.309850] usb 1-1: Manufacturer: bar > [ 757.309851] usb 1-1: SerialNumber: 0 to make the bug easier to reproduce ,I have changed the initial value of segnum: diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 8dac1edc74d4..92a60e89b459 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -1192,7 +1192,7 @@ static int vhci_start(struct usb_hcd *hcd) vdev->rhport = rhport; } - atomic_set(&vhci_hcd->seqnum, 0); + atomic_set(&vhci_hcd->seqnum, 2147483646); hcd->power_budget = 0; /* no limit */ hcd->uses_new_polling = 1; On Fri, Jan 10, 2025 at 12:02 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 1/3/25 08:18, xndcn wrote: > > Thanks. > > > >> How did you find the problem? > >> Why does it make sense to cast it to u32? > > > > After running with usbip enough time, I happened to see logs like this: > >> [ 293.863125] vhci_hcd vhci_hcd.0: Device attached > >> [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd > >> [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 > >> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > >> [ 294.204850] vhci_hcd: stop threads > >> [ 294.204851] vhci_hcd: release socket > >> [ 294.204853] vhci_hcd: disconnect device > > > > Then I notice that on 64bit platform, when > > atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or > > 0x80000000), > > priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be > > extends to 0xffffffff80000000 > > So we can fix the issue by cast it to u32. > > > > Can you send me the dmesg without and with your patch? > > thanks, > -- Shuah >
On 12/31/24 09:15, Xiong Nandi wrote: > The atomic_inc_return function returns an int, while priv->seqnum is an > unsigned long. So we must cast the result to u32 to prevent potential > sign extension and size mismatch issues. > > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > --- > drivers/usb/usbip/vhci_hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > index b03e5021c25b..f3f260e01791 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > spin_lock_irqsave(&vdev->priv_lock, flags); > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); > if (priv->seqnum == 0xffff) > dev_info(&urb->dev->dev, "seqnum max\n"); > Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index b03e5021c25b..f3f260e01791 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) spin_lock_irqsave(&vdev->priv_lock, flags); - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); if (priv->seqnum == 0xffff) dev_info(&urb->dev->dev, "seqnum max\n");
The atomic_inc_return function returns an int, while priv->seqnum is an unsigned long. So we must cast the result to u32 to prevent potential sign extension and size mismatch issues. Signed-off-by: Xiong Nandi <xndchn@gmail.com> --- drivers/usb/usbip/vhci_hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)