linux-kernel - Re: Kernel panic while doing vfio-pci hot-plug/unplug test

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: <alpine.DEB.2.21.1910241001250.1852@nanos.tec.linutronix.de>
Date:   Thu, 24 Oct 2019 10:07:23 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Matthew Wilcox <willy@...radead.org>
cc:     Bjorn Helgaas <helgaas@...nel.org>,
        Xiang Zheng <zhengxiang9@...wei.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        mingo@...hat.com, peterz@...radead.org, alex.williamson@...hat.com,
        Wang Haibin <wanghaibin.wang@...wei.com>,
        Guoheyi <guoheyi@...wei.com>,
        yebiaoxiang <yebiaoxiang@...wei.com>
Subject: Re: Kernel panic while doing vfio-pci hot-plug/unplug test

On Wed, 23 Oct 2019, Matthew Wilcox wrote:
> Some problems I see:
> 
> 1. Commit df65c1bcd9b7b639177a5a15da1b8dc3bee4f5fa (tglx) says:
> 
>     x86/PCI: Select CONFIG_PCI_LOCKLESS_CONFIG
>     
>     All x86 PCI configuration space accessors have either their own
>     serialization or can operate completely lockless (ECAM).
>     
>     Disable the global lock in the generic PCI configuration space accessors.
> 
> The concept behind this patch is broken.  We still need to lock out
> config space accesses when devices are undergoing D-state transitions.
> I would suggest that for the contention case that tglx is concerned about,
> we should have a pci_bus_read_config_unlocked_##size set of functions
> which can be used for devices we know never go into D states.

I don't think that it's broken. A D-state transition has to make sure that
the rest of stuff which might be touching the config space is
quiescent. pci_lock cannot provide that protection
 
> 
> 2. Commit a2e27787f893621c5a6b865acf6b7766f8671328 (jan kiszka)
>    exports pci_lock.  I think this is a mistake; at best there should be
>    accessors for the pci_lock.  But I don't understand why it needs to
>    exclude PCI config space changes throughout pci_check_and_set_intx_mask().
>    Why can it not do:
> 
> -	bus->ops->read(bus, dev->devfn, PCI_COMMAND, 4, &cmd_status_dword);
> +	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);

Hmm. Need to look closer on that.
 
> 3. I don't understand why 511dd98ce8cf6dc4f8f2cb32a8af31ce9f4ba4a1
>    changed pci_lock to be a raw spinlock.  The patch description
>    essentially says "We need it for RT" which isn't terribly helpful.

Yes, I could slap myself for writing such a useless changelog. The reason
why it is a raw spinlock is that config space access happens from very low
level contexts, which require to have interrupts disabled even on RT,
e.g. from the guts of the interrupt code.

> 4. Finally, getting back to the original problem report here, I wouldn't
>    write this code this way today.  There's no reason not to use the
>    regular add_wait_queue etc.  BUT!  Why are we using this custom locking
>    mechanism?  It pretty much screams to me of an rwsem (reads/writes
>    of config space take it for read; changes to config space accesses
>    (disabling and changing of accessor methods) take it for write.

You cannot use a RWSEM as low level interrupt code needs to access the
config space with interrupts disabled and raw spinlocks held, e.g. to
fiddle with the interrupt and MSI stuff.

Thanks,

	tglx