linux-kernel - Re: [PATCH 1/7] PCI: Make sriov work with hotplug remove

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: <CAE9FiQWbmO+FFsE06eQ=-PVNtdaTG3EsqXNXdUCxb9YxtPS8Cw@mail.gmail.com>
Date:	Mon, 23 Jan 2012 10:30:46 -0800
From:	Yinghai Lu <yinghai@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
	Matthew Wilcox <matthew@....cx>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] PCI: Make sriov work with hotplug remove

On Mon, Jan 23, 2012 at 8:06 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Sat, Jan 21, 2012 at 1:52 AM, Yinghai Lu <yinghai@...nel.org> wrote:
>>
>> +       /*
>> +        * pci_stop_bus_device(dev) will not remove dev from bus->devices list,
>> +        *  so We don't need use _safe version for_each here.
>> +        * Also _safe version has problem when pci_stop_bus_device() for PF try
>> +        *  to remove VFs.
>> +        */
>> +       for (l = head->next; l != head;) {
>
> That's crazy. Why would you open-code this? Why isn't it just a
> "list_for_each()"?

I have previous version used list_for_each(), but Kenji thought we
should open version because it could be clear that l is updated in the
loop.

>
> And what are the problems with the safe version? If the safe version
> doesn't work, then something is *seriously* wrong with the list.

in list_for_each_safe()

#define list_for_each_safe(pos, n, head) \
        for (pos = (head)->next, n = pos->next; pos != (head); \
                pos = n, n = pos->next)

n is saved before, and safe only mean pos could be freed from the
list, but n still can be used for next loop.

in our case, the list have PF and several VFs, when
pci_stop_bus_device() is called for PF, pos are still valid, but
VFs are removed from the list. so n will not be valid.

>
>> +               struct pci_dev *dev = pci_dev_b(l);
>> +
>> +               /*
>> +                * VFs are removed by pci_remove_bus_device() in the
>> +                *  pci_stop_bus_devices() code path for PF.
>> +                *  aka, bus->devices get updated in the process.
>> +                * barrier() will make sure we get right next from that list.
>> +                */
>> +               if (!dev->is_virtfn) {
>> +                       pci_stop_bus_device(dev);
>> +                       barrier();
>> +               }
>
> And this is just insanity. The "barrier()" cannot *possibly* do
> anything sane. If it really makes a difference, there is again some
> serious problem with the whole f*cking thing.
>
> NAK on the patch until sanity is restored. This is just total voodoo
> programming.

Sorry for that.

Can you please check V1 version ?

https://lkml.org/lkml/2011/10/15/141
or from attached one.

Thanks

Yinghai

View attachment "pci_001_debug_sriov_hot_remove.patch" of type "text/x-patch" (6117 bytes)