Re: Broken pci_block_user_cfg_access interface

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-08-25 11:40, Michael S. Tsirkin wrote:
> On Thu, Aug 25, 2011 at 11:19:54AM +0200, Jan Kiszka wrote:
>> On 2011-08-24 17:02, Brian King wrote:
>>> On 08/24/2011 05:43 AM, Jan Kiszka wrote:
>>>> Hi,
>>>>
>>>> trying to port the generic device interrupt masking pattern of
>>>> uio_pci_generic to KVM's device assignment code, I stumbled over some
>>>> fundamental problem with the current pci_block/unblock_user_cfg_access
>>>> interface: it does not provide any synchronization between blocking
>>>> sides. This allows user space to trigger a kernel BUG, just run two
>>>>
>>>> while true; do echo 1 > /sys/bus/pci/devices/<some-device>/reset; done
>>>>
>>>> loops in parallel and watch the kernel oops.
>>>>
>>>> Instead of some funky open-coded locking mechanism, we would rather need
>>>> a plain mutex across both the user space access (via sysfs) and the
>>>> sections guarded by pci_block/unblock_user_cfg_access so far. But I'm
>>>> not sure which of them already allow sleeping, specifically if the IPR
>>>> driver would be fine with such a change. Can someone in the CC list
>>>> comment on this?
>>>
>>> The ipr driver calls pci_block/unblock_user_cfg_access from interrupt
>>> context, so a mutex won't work.
>>
>> Ugh. What precisely does it have to do with the config space while
>> running inside an IRQ handler (or holding a lock that synchronizes it
>> with such a handler)?
>>
>>> When the pci_block/unblock API was
>>> originally added, it did not have the checking it has today to detect
>>> if it is being called nested. This was added some time later. The
>>
>> For a reason...
>>
>>> API that works best for the ipr driver is to allow for many block calls,
>>> but a single unblock call unblocks access. It seems like what might
>>> work well in the case above is a block count. Each call to pci_block
>>> increments a count. Each pci_unblock decrements the count and only
>>> actually do the unblock if the count drops to zero. It should be reasonably
>>> simple for ipr to use that sort of an API as well.
>>
>> That will just paper over the underlying bug: multiple kernel users (!=
>> sysfs access) fiddle with the config space in an unsynchronized fashion.
>> Think of sysfs-triggered pci_reset_function while your ipr driver does
>> its accesses.
>>
>> So it's pointless to tweak the current pci_block semantics, we rather
>> need to establish a new mechanism that synchronizes *all* users of the
>> config space.
>>
>> Jan
> 
> It does look like all of the problems are actually around reset.
> So maybe all we need to do is synchronize the sysfs-triggered
> pci_reset_function with pci_block/unblock_user_cfg_access?
> 
> In other words, when reset is triggered from sysfs, it
> should obey pci_block/unblock_user_cfg_access
> restrictions?
> 
> It does not look like reset needs to sleep, so fixing
> that should not be hard, right?

It obviously does need to sleep (wait). I just don't know if blocking
config space access is required during that phase, but I wouldn't be
surprised if it was.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux