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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAD=FV=XGBfkLQ2N3dr3smhMDb+ze-XpbHjd7EChAByGNwJOnOw@mail.gmail.com>
Date: Thu, 14 Nov 2024 14:37:10 -0800
From: Doug Anderson <dianders@...omium.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.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

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>