linux-kernel - Re: [PATCH v2 1/2] USB: make single lock for all usb dynamic id lists

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] [day] [month] [year] [list]

Message-ID: <2024111554-elves-crumpled-0a24@gregkh>
Date: Fri, 15 Nov 2024 17:46:00 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Doug Anderson <dianders@...omium.org>
Cc: linux-usb@...r.kernel.org, stern@...land.harvard.edu,
	linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>,
	Herve Codina <herve.codina@...tlin.com>,
	Rob Herring <robh@...nel.org>,
	Grant Grundler <grundler@...omium.org>,
	Oliver Neukum <oneukum@...e.com>, Yajun Deng <yajun.deng@...ux.dev>
Subject: Re: [PATCH v2 1/2] USB: make single lock for all usb dynamic id lists

On Thu, Nov 14, 2024 at 02:37:10PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 12, 2024 at 10:49 PM Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> >
> > There are a number of places where we accidentally pass in a constant
> > structure to later cast it off to a dynamic one, and then attempt to
> > grab a lock on it, which is not a good idea.  To help resolve this, move
> > the dynamic id lock out of the dynamic id structure for the driver and
> > into one single lock for all USB dynamic ids.  As this lock should never
> > have any real contention (it's only every accessed when a device is
> 
> nit: s/every/ever/
> 
> 
> > added or removed, which is always serialized) there should not be any
> > difference except for some memory savings.
> >
> > Note, this just converts the existing use of the dynamic id lock to the
> > new static lock, there is one place that is accessing the dynamic id
> > list without grabbing the lock, that will be fixed up in a follow-on
> > change.
> >
> > Cc: Johan Hovold <johan@...nel.org>
> > Cc: Herve Codina <herve.codina@...tlin.com>
> > Cc: Rob Herring <robh@...nel.org>
> > Cc: Alan Stern <stern@...land.harvard.edu>
> > Cc: Grant Grundler <grundler@...omium.org>
> > Cc: Oliver Neukum <oneukum@...e.com>
> > Cc: Yajun Deng <yajun.deng@...ux.dev>
> > Cc: Douglas Anderson <dianders@...omium.org>
> > Cc: linux-usb@...r.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > ---
> > v2: - change to a mutex
> >     - strip out of larger series
> >
> >  drivers/usb/common/common.c     |  3 +++
> >  drivers/usb/core/driver.c       | 15 +++++----------
> >  drivers/usb/serial/bus.c        |  4 +---
> >  drivers/usb/serial/usb-serial.c |  4 +---
> >  include/linux/usb.h             |  2 +-
> >  5 files changed, 11 insertions(+), 17 deletions(-)
> 
> I'm not familiar enough with the code to confirm with 100% certainty
> your assertions that there won't be any contention problems with this
> lock. However, your argument sounds reasonable to me and, since you
> are much more familiar with the subsystem, I'll believe you. :-)
> 
> I would have a slight concern that you are changing a "spin_lock" to a
> "mutex", which doesn't seem to be talked about in the patch
> description. This is likely to be fine given that all of the users are
> "spin_lock" and not "spin_lock_irq" or "spin_lock_irqsave", but it
> still makes me worried that there's some random bit of code somewhere
> that calls one of these functions while sleeping is disabled. I guess
> that's not likely.
> 
> In any case, this seems OK to me assuming it tests well.
> 
> Reviewed-by: Douglas Anderson <dianders@...omium.org>
> 

THanks for the reviews!