| 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
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(-)