Re: [PATCH 2/4] USB: wdm: close race between wdm_open and wdm_wwan_port_stop
From: Alan Stern <stern@rowland.harvard.edu>
Date: 2025-03-31 14:37:59
Also in:
linux-usb
On Mon, Mar 31, 2025 at 03:25:02PM +0200, Oliver Neukum wrote:
quoted hunk ↗ jump to hunk
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