[PATCH 0/4] USB: wdm: fix WWAN integration issue

[PATCH 0/4] USB: wdm: fix WWAN integration issue

[Oliver Neukum]

The original integration of WWAN left a few race conditions
and deficiencies in error handling

[PATCH 1/4] USB: wdm: handle IO errors in wdm_wwan_port_start

[Oliver Neukum]

In case submitting the URB fails we must undo
what we've done so far.

Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 856488a7cb6b..12038aa43942 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -831,6 +831,7 @@ static struct usb_class_driver wdm_class = {
 static int wdm_wwan_port_start(struct wwan_port *port)
 {
 	struct wdm_device *desc = wwan_port_get_drvdata(port);
+	int rv;
 
 	/* The interface is both exposed via the WWAN framework and as a
 	 * legacy usbmisc chardev. If chardev is already open, just fail
@@ -850,7 +851,15 @@ static int wdm_wwan_port_start(struct wwan_port *port)
 	wwan_port_txon(port);
 
 	/* Start getting events */
-	return usb_submit_urb(desc->validity, GFP_KERNEL);
+	rv = usb_submit_urb(desc->validity, GFP_KERNEL);
+	if (rv < 0) {
+		wwan_port_txoff(port);
+		desc->manage_power(desc->intf, 0);
+		/* this must be last lest we race with chardev open */
+		clear_bit(WDM_WWAN_IN_USE, &desc->flags);
+	}
+
+	return rv;
 }
 
 static void wdm_wwan_port_stop(struct wwan_port *port)
-- 
2.49.0

[PATCH 2/4] USB: wdm: close race between wdm_open and wdm_wwan_port_stop

[Oliver Neukum]

Clearing WDM_WWAN_IN_USE must be the last action or
we can open a chardev whose URBs are still poisoned

Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 12038aa43942..e67844618da6 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -870,8 +870,9 @@ static void wdm_wwan_port_stop(struct wwan_port *port)
 	poison_urbs(desc);
 	desc->manage_power(desc->intf, 0);
 	clear_bit(WDM_READ, &desc->flags);
-	clear_bit(WDM_WWAN_IN_USE, &desc->flags);
 	unpoison_urbs(desc);
+	/* this must be last lest we open a poisoned device */
+	clear_bit(WDM_WWAN_IN_USE, &desc->flags);
 }
 
 static void wdm_wwan_port_tx_complete(struct urb *urb)
-- 
2.49.0

[PATCH 3/4] USB: wdm: wdm_wwan_port_tx_complete mutex in atomic context

[Oliver Neukum]

wdm_wwan_port_tx_complete is called from a completion
handler with irqs disabled and possible in IRQ context
usb_autopm_put_interface can take a mutex.
Hence usb_autopm_put_interface_async must be used.

Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index e67844618da6..f50c3ad86eca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -880,7 +880,7 @@ static void wdm_wwan_port_tx_complete(struct urb *urb)
 	struct sk_buff *skb = urb->context;
 	struct wdm_device *desc = skb_shinfo(skb)->destructor_arg;
 
-	usb_autopm_put_interface(desc->intf);
+	usb_autopm_put_interface_async(desc->intf);
 	wwan_port_txon(desc->wwanp);
 	kfree_skb(skb);
 }
-- 
2.49.0

[PATCH 4/4] USB: wdm: add annotation

[Oliver Neukum]

This is not understandable without a comment on endianness

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index f50c3ad86eca..caf721c11e9c 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -910,7 +910,7 @@ static int wdm_wwan_port_tx(struct wwan_port *port, struct sk_buff *skb)
 	req->bRequestType = (USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE);
 	req->bRequest = USB_CDC_SEND_ENCAPSULATED_COMMAND;
 	req->wValue = 0;
-	req->wIndex = desc->inum;
+	req->wIndex = desc->inum; /* already converted */
 	req->wLength = cpu_to_le16(skb->len);
 
 	skb_shinfo(skb)->destructor_arg = desc;
-- 
2.49.0

Re: [PATCH 2/4] USB: wdm: close race between wdm_open and wdm_wwan_port_stop

[Alan Stern]

On Mon, Mar 31, 2025 at 03:25:02PM +0200, Oliver Neukum wrote:
> Clearing WDM_WWAN_IN_USE must be the last action or
> we can open a chardev whose URBs are still poisoned
> 
> Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration")
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/class/cdc-wdm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 12038aa43942..e67844618da6 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -870,8 +870,9 @@ static void wdm_wwan_port_stop(struct wwan_port *port)
>  	poison_urbs(desc);
>  	desc->manage_power(desc->intf, 0);
>  	clear_bit(WDM_READ, &desc->flags);
> -	clear_bit(WDM_WWAN_IN_USE, &desc->flags);
>  	unpoison_urbs(desc);
> +	/* this must be last lest we open a poisoned device */
> +	clear_bit(WDM_WWAN_IN_USE, &desc->flags);
>  }

This is a good example of a place where a memory barrier is needed.  
Neither unpoison_urbs() nor clear_bit() includes an explicit memory 
barrier.  So even though patch ensures that unpoison_urb() occurs before 
clear_bit() in the source code, there is still no guarantee that a CPU 
will execute them in that order.  Or even if they are executed in order, 
there is no guarantee that a different CPU will see their effects 
occurring in that order.

In this case you almost certainly need to have an smp_wmb() between the 
two statements, with a corresponding smp_rmb() (or equivalent) in the 
code that checks whether the WDM_WWAIN_IN_USE flag is set.

Alan Stern