linux-kernel - Re: WARNING in cm109_urb_irq_callback/usb_submit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <04a011b5-a7fa-4270-a072-365b5abd2aec@suse.com>
Date: Thu, 27 Mar 2025 12:42:41 +0100
From: Oliver Neukum <oneukum@...e.com>
To: Alan Stern <stern@...land.harvard.edu>, Oliver Neukum <oneukum@...e.com>
Cc: 白烁冉 <baishuoran@...eu.edu.cn>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>, Kun Hu <huk23@...udan.edu.cn>,
 Jiaji Qin <jjtan24@...udan.edu.cn>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
 syzkaller@...glegroups.com
Subject: Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

Hi,

On 20.03.25 18:25, Alan Stern wrote:

>> static void cm109_stop_traffic(struct cm109_dev *dev)
>> {
>>          dev->shutdown = 1;
>>          /*
>>           * Make sure other CPUs see this
>>           */
>>          smp_wmb();
>>          usb_kill_urb(dev->urb_ctl);
>>          usb_kill_urb(dev->urb_irq);
>>          cm109_toggle_buzzer_sync(dev, 0);
>>          dev->shutdown = 0;
>>          smp_wmb();
> 
> I don't know anything about this driver, but the placement of the second
> smp_wmb() looks odd.  Should it really come before the line that sets

Indeed. This driver is not written for comprehension. As far as I can tell
it is not necessary at all. You need to set shutdown to zero before you
resubmit the URBs. But I don't see how the barrier helps with that.

> dev->shutdown to 0?  In general, smp_wmb() is used to separate two sets
> of stores; if it comes after all the relevant stores have been performed
> then it won't accomplish anything.

Don't we guarantee an interaction between smp_wmb() and taking a spinlock?
> 
>> }
>>
>> This driver has a tough job as the two completion
>> handlers submitted each other's as well as their own
>> URBs based on the data they get.
>> That scheme is rather complex, but as far as I can tell correct,
>> but you need to test that flag everywhere.
> 
> However, it's quite noticeable that the code you want to change in
> cm109_submit_buzz_toggle() doesn't have any memory barriers to pair with
> the smb_wmb()'s above.  Shouldn't there at least be an smp_rmb() after
> you read dev->shutdown?

I think this driver assumes that the ctl_submit_lock spinlock makes
it safe.

> As long you're updating the synchronization, it might be a good idea to
> also improve the comment describing the memory barriers.  "Make sure
> other CPUs see this" doesn't mean anything -- of course all the other
> CPUs will eventually see the changes made here.  The point is that they
> should see the new value of dev->shutdown before seeing the final
> completion of the two URBs.  Also, the comment should say which other
> memory barriers pair with the ones here.

Before the completion? AFAICT they need to see it before they try
to submit an URB.

	Regards
		Oliver