[PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues

[PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues

[Xiong Nandi]

Xiong Nandi (2):
  usbip: Fix seqnum sign extension issue in vhci_tx_urb
  usbip: Correct format specifier for seqnum from %d to %u

 drivers/usb/usbip/stub_rx.c  | 2 +-
 drivers/usb/usbip/stub_tx.c  | 2 +-
 drivers/usb/usbip/vhci_hcd.c | 2 +-
 drivers/usb/usbip/vhci_rx.c  | 6 +++---
 drivers/usb/usbip/vudc_tx.c  | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[Xiong Nandi]

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");
 
-- 
2.25.1

[PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u

[Xiong Nandi]

The seqnum field in USBIP protocol is an unsigned value.
So we fix the format specifier from %d to %u to correctly
display the value.

Signed-off-by: Xiong Nandi <xndchn@gmail.com>
---
 drivers/usb/usbip/stub_rx.c | 2 +-
 drivers/usb/usbip/stub_tx.c | 2 +-
 drivers/usb/usbip/vhci_rx.c | 6 +++---
 drivers/usb/usbip/vudc_tx.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 6338d818bc8b..9aa30ef76f3b 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
 		return 0;
 	}
 
-	usbip_dbg_stub_rx("seqnum %d is not pending\n",
+	usbip_dbg_stub_rx("seqnum %u is not pending\n",
 			  pdu->u.cmd_unlink.seqnum);
 
 	/*
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index b1c2f6781cb3..7eb2e074012a 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 
 		/* 1. setup usbip_header */
 		setup_ret_submit_pdu(&pdu_header, urb);
-		usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+		usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
 				  pdu_header.base.seqnum);
 
 		if (priv->sgl) {
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index 7f2d1c241559..a75f4a898a41 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 	spin_unlock_irqrestore(&vdev->priv_lock, flags);
 
 	if (!urb) {
-		pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
+		pr_err("cannot find a urb of seqnum %u max seqnum %u\n",
 			pdu->base.seqnum,
 			atomic_read(&vhci_hcd->seqnum));
 		usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
@@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
 		 * already received the result of its submit result and gave
 		 * back the URB.
 		 */
-		pr_info("the urb (seqnum %d) was already given back\n",
+		pr_info("the urb (seqnum %u) was already given back\n",
 			pdu->base.seqnum);
 	} else {
-		usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum);
+		usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum);
 
 		/* If unlink is successful, status is -ECONNRESET */
 		urb->status = pdu->u.ret_unlink.status;
diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c
index 3ccb17c3e840..30c11bf9f4e7 100644
--- a/drivers/usb/usbip/vudc_tx.c
+++ b/drivers/usb/usbip/vudc_tx.c
@@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p)
 
 	/* 1. setup usbip_header */
 	setup_ret_submit_pdu(&pdu_header, urb_p);
-	usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+	usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
 			  pdu_header.base.seqnum);
 	usbip_header_correct_endian(&pdu_header, 1);
 
-- 
2.25.1

Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[Shuah Khan]

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

Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u

[Shuah Khan]

On 12/31/24 09:15, Xiong Nandi wrote:
> The seqnum field in USBIP protocol is an unsigned value.
> So we fix the format specifier from %d to %u to correctly
> display the value.
> 

How did you find the problem? Include log from the tool
or compiler output.

> Signed-off-by: Xiong Nandi <xndchn@gmail.com>
> ---
>   drivers/usb/usbip/stub_rx.c | 2 +-
>   drivers/usb/usbip/stub_tx.c | 2 +-
>   drivers/usb/usbip/vhci_rx.c | 6 +++---
>   drivers/usb/usbip/vudc_tx.c | 2 +-
>   4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 6338d818bc8b..9aa30ef76f3b 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
>   		return 0;
>   	}
>   
> -	usbip_dbg_stub_rx("seqnum %d is not pending\n",
> +	usbip_dbg_stub_rx("seqnum %u is not pending\n",
>   			  pdu->u.cmd_unlink.seqnum);

seqnum is unsigned long - don't you have to use %ul?
>   
>   	/*
> diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> index b1c2f6781cb3..7eb2e074012a 100644
> --- a/drivers/usb/usbip/stub_tx.c
> +++ b/drivers/usb/usbip/stub_tx.c
> @@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev)
>   
>   		/* 1. setup usbip_header */
>   		setup_ret_submit_pdu(&pdu_header, urb);
> -		usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> +		usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
>   				  pdu_header.base.seqnum);
>   
>   		if (priv->sgl) {
> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index 7f2d1c241559..a75f4a898a41 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
>   	spin_unlock_irqrestore(&vdev->priv_lock, flags);
>   
>   	if (!urb) {
> -		pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
> +		pr_err("cannot find a urb of seqnum %u max seqnum %u\n",
>   			pdu->base.seqnum,
>   			atomic_read(&vhci_hcd->seqnum));
>   		usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> @@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
>   		 * already received the result of its submit result and gave
>   		 * back the URB.
>   		 */
> -		pr_info("the urb (seqnum %d) was already given back\n",
> +		pr_info("the urb (seqnum %u) was already given back\n",
>   			pdu->base.seqnum);
>   	} else {
> -		usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum);
> +		usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum);
>   
>   		/* If unlink is successful, status is -ECONNRESET */
>   		urb->status = pdu->u.ret_unlink.status;
> diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c
> index 3ccb17c3e840..30c11bf9f4e7 100644
> --- a/drivers/usb/usbip/vudc_tx.c
> +++ b/drivers/usb/usbip/vudc_tx.c
> @@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p)
>   
>   	/* 1. setup usbip_header */
>   	setup_ret_submit_pdu(&pdu_header, urb_p);
> -	usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> +	usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
>   			  pdu_header.base.seqnum);
>   	usbip_header_correct_endian(&pdu_header, 1);
>   
thanks,
-- Shuah

Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[xndcn]

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

Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u

[xndcn]

Thanks.

> How did you find the problem? Include log from the tool
> or compiler output.

As said in [Patch 1/2], I saw this log:
> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648
then I found there are also logs with wrong format specifier for seqnum

> seqnum is unsigned long - don't you have to use %ul?
pdu->u.cmd_unlink.seqnum is defined as u32 actually.
In usbip driver, all seqnum in protocol struct (struct
usbip_header_basic and struct usbip_header_cmd_unlink), is defined as
u32.
In other driver specific priv struct, seqnum is defined as unsigned long.
It seems only the u32 seqnum have wrong format specifier

On Fri, Jan 3, 2025 at 6:09 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 12/31/24 09:15, Xiong Nandi wrote:
> > The seqnum field in USBIP protocol is an unsigned value.
> > So we fix the format specifier from %d to %u to correctly
> > display the value.
> >
>
> How did you find the problem? Include log from the tool
> or compiler output.
>
> > Signed-off-by: Xiong Nandi <xndchn@gmail.com>
> > ---
> >   drivers/usb/usbip/stub_rx.c | 2 +-
> >   drivers/usb/usbip/stub_tx.c | 2 +-
> >   drivers/usb/usbip/vhci_rx.c | 6 +++---
> >   drivers/usb/usbip/vudc_tx.c | 2 +-
> >   4 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> > index 6338d818bc8b..9aa30ef76f3b 100644
> > --- a/drivers/usb/usbip/stub_rx.c
> > +++ b/drivers/usb/usbip/stub_rx.c
> > @@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
> >               return 0;
> >       }
> >
> > -     usbip_dbg_stub_rx("seqnum %d is not pending\n",
> > +     usbip_dbg_stub_rx("seqnum %u is not pending\n",
> >                         pdu->u.cmd_unlink.seqnum);
>
> seqnum is unsigned long - don't you have to use %ul?
> >
> >       /*
> > diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> > index b1c2f6781cb3..7eb2e074012a 100644
> > --- a/drivers/usb/usbip/stub_tx.c
> > +++ b/drivers/usb/usbip/stub_tx.c
> > @@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> >
> >               /* 1. setup usbip_header */
> >               setup_ret_submit_pdu(&pdu_header, urb);
> > -             usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> > +             usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
> >                                 pdu_header.base.seqnum);
> >
> >               if (priv->sgl) {
> > diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> > index 7f2d1c241559..a75f4a898a41 100644
> > --- a/drivers/usb/usbip/vhci_rx.c
> > +++ b/drivers/usb/usbip/vhci_rx.c
> > @@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
> >       spin_unlock_irqrestore(&vdev->priv_lock, flags);
> >
> >       if (!urb) {
> > -             pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
> > +             pr_err("cannot find a urb of seqnum %u max seqnum %u\n",
> >                       pdu->base.seqnum,
> >                       atomic_read(&vhci_hcd->seqnum));
> >               usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> > @@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
> >                * already received the result of its submit result and gave
> >                * back the URB.
> >                */
> > -             pr_info("the urb (seqnum %d) was already given back\n",
> > +             pr_info("the urb (seqnum %u) was already given back\n",
> >                       pdu->base.seqnum);
> >       } else {
> > -             usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum);
> > +             usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum);
> >
> >               /* If unlink is successful, status is -ECONNRESET */
> >               urb->status = pdu->u.ret_unlink.status;
> > diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c
> > index 3ccb17c3e840..30c11bf9f4e7 100644
> > --- a/drivers/usb/usbip/vudc_tx.c
> > +++ b/drivers/usb/usbip/vudc_tx.c
> > @@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p)
> >
> >       /* 1. setup usbip_header */
> >       setup_ret_submit_pdu(&pdu_header, urb_p);
> > -     usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> > +     usbip_dbg_stub_tx("setup txdata seqnum: %u\n",
> >                         pdu_header.base.seqnum);
> >       usbip_header_correct_endian(&pdu_header, 1);
> >
> thanks,
> -- Shuah

Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[Shuah Khan]

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

Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[xndcn]

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
>

Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

[Shuah Khan]

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

Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u

[Shuah Khan]

On 12/31/24 09:15, Xiong Nandi wrote:
> The seqnum field in USBIP protocol is an unsigned value.
> So we fix the format specifier from %d to %u to correctly
> display the value.
> 
> Signed-off-by: Xiong Nandi <xndchn@gmail.com>
> ---
>   drivers/usb/usbip/stub_rx.c | 2 +-
>   drivers/usb/usbip/stub_tx.c | 2 +-
>   drivers/usb/usbip/vhci_rx.c | 6 +++---
>   drivers/usb/usbip/vudc_tx.c | 2 +-
>   4 files changed, 6 insertions(+), 6 deletions(-)
> 

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah