linux-kernel - Re: [PATCH v2] pciehp: fix a race between pciehp and removing operations by sysfs

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: <20190811080703.qfnlzfutgamoxzti@wunner.de>
Date:   Sun, 11 Aug 2019 10:07:03 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Xiongfeng Wang <wangxiongfeng2@...wei.com>
Cc:     helgaas@...nel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pciehp: fix a race between pciehp and removing
 operations by sysfs

On Mon, Aug 05, 2019 at 08:32:58PM +0800, Xiongfeng Wang wrote:
> When we remove a slot by sysfs.
> 'pci_stop_and_remove_bus_device_locked()' will be called. This function
> will get the global mutex lock 'pci_rescan_remove_lock', and remove the
> slot. If the irq thread 'pciehp_ist' is still running, we will wait
> until it exits.
> 
> If a pciehp interrupt happens immediately after we remove the slot by
> sysfs, but before we free the pciehp irq in
> 'pci_stop_and_remove_bus_device_locked()'. 'pciehp_ist' will hung
> because the global mutex lock 'pci_rescan_remove_lock' is held by the
> sysfs operation. But the sysfs operation is waiting for the pciehp irq
> thread 'pciehp_ist' ends. Then a hung task occurs.
> 
> So this two kinds of operation, removing the slot triggered by pciehp
> interrupt and removing through 'sysfs', should not be excuted at the
> same time. This patch add a global variable to mark that one of these
> operations is under processing. When this variable is set,  if another
> operation is requested, it will be rejected.

It seems this patch involves an ABI change wherein "remove" as documented
in Documentation/ABI/testing/sysfs-bus-pci may now fail and need a retry,
possibly breaking existing scripts which write to this file.  ABI changes
are fairly problematic.

The return value -EWOULDBLOCK (which is identical to -EAGAIN) might be
more appropriate than -EINVAL.

Another problem is that this patch only addresses deadlocks occurring
because of a "remove" via sysfs and a simultaneous surprise removal
(or button press removal).  However the same kind of deadlock may
occur because of two simultaneous surprise removals if one of the
two devices is a parent of the other.  It would be better to have
a solution which addresses all types of deadlocks caused by the
pci_rescan_remove_lock().  I provided you with a suggestion in this
e-mail:

https://lore.kernel.org/linux-pci/20190805114053.srbngho3wbziy2uy@wunner.de/

   "What you can do is add a flag to struct pci_dev (or the priv_flags
    embedded therein) to indicate that a device is about to be removed.
    Set this flag on all children of the device being removed before
    acquiring pci_lock_rescan_remove() and avoid taking that lock in
    pciehp_unconfigure_device() if the flag is set on the hotplug port.

    But again, that approach is just a band-aid and the real fix is to
    unbind devices without holding the lock."

Thanks,

Lukas