linux-kernel - Re: [RFC PATCH 0/6] pci: Rework is_added race fix and address bridge enable races

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]

Date:   Fri, 17 Aug 2018 15:03:49 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Bjorn Helgaas <helgaas@...nel.org>, linux-pci@...r.kernel.org
Cc:     Hari Vyas <hari.vyas@...adcom.com>, Ray Jui <ray.jui@...adcom.com>,
        Srinath Mannam <srinath.mannam@...adcom.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jens Axboe <axboe@...nel.dk>, Lukas Wunner <lukas@...ner.de>,
        Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        Marta Rybczynska <mrybczyn@...ray.eu>,
        Pierre-Yves Kerbrat <pkerbrat@...ray.eu>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/6] pci: Rework is_added race fix and address
 bridge enable races

On Fri, 2018-08-17 at 14:48 +1000, Benjamin Herrenschmidt wrote:
> The second part aims at fixing the enable/disable/set_master races,
> and does so by providing a framework for future device state locking
> issues.
> 
> It introduces a pci_dev->state_mutex which is used at a lower level
> than the device_lock (the device lock isn't suitable, as explained
> in the cset comments) and uses it to protect enablement and set_master.

As discussed in the series, I'm not using the device_lock because I
don't like it creeping out of drivers/base too much unless you
explicitly try to lock against concurrent add/remove.

That being said, if we decided we prefer using it to solve the
enable/disable race, then we have to be careful of a few things:

 - Driver callbacks hold it, so we can't take it from within
pci_enable_device(), pci_set_master() etc... themselves. We'll have to
assume the caller has it

 - The above means auditing callers of these various APIs that might be
calling them from outside of a driver callback and add the necessary
lock

 - We need to take great care about the possibility of the parent
device(s) lock being held. It can happen for USB for example. Now we
don't have USB->PCI adapters that I know of that uses the PCI stack in
Linux but generally assuming your parent lock isn't held is risky. So I
would be a bit weary of walking up the bridge chain and taking it
unconditionally.

Alternatively, we could hijack an existing global lock, for example
mvoe the bridge_mutex outside of ACPI and use it for the upwards bridge
walk, and ignore the other possible races with enable/disable.

Cheers,
Ben.