Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651673 > unrolled thread

Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-05-27 03:30 +0200
    Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses Bjorn Helgaas <helgaas@kernel.org> - 2017-05-31 01:10 +0200
      Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses Jakub Kicinski <kubakici@wp.pl> - 2017-05-31 01:40 +0200

#1651673 — Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:
> On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:
> > PCI core sets the driver pointer before calling ->probe() and only
> > clears it after ->remove().  This means driver's ->sriov_configure()
> > callback will happily race with probe() and remove(), most likely
> > leading to BUGs, since drivers don't expect this.  
> 
> I guess you're referring to the pci_dev->driver pointer set by
> local_pci_probe(), and this is important because sriov_numvfs_store()
> checks that pointer, right?

Yes, exactly.  I initially thought this is how the safety of sriov
callback may have been ensured, but since the order of
local_pci_probe() and the assignment is what it is, it can't.

[toc] | [next] | [standalone]

#1653707

On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:
> > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:
> > > PCI core sets the driver pointer before calling ->probe() and only
> > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > callback will happily race with probe() and remove(), most likely
> > > leading to BUGs, since drivers don't expect this.  
> > 
> > I guess you're referring to the pci_dev->driver pointer set by
> > local_pci_probe(), and this is important because sriov_numvfs_store()
> > checks that pointer, right?
> 
> Yes, exactly.  I initially thought this is how the safety of sriov
> callback may have been ensured, but since the order of
> local_pci_probe() and the assignment is what it is, it can't.

Right.  I was hoping other subsystems would establish a convention
about whether we set the ->driver pointer before or after calling the
driver probe() method, but if there is one, I don't see it.
local_pci_probe() and really_probe() set ->driver first, but
pnp_device_probe() calls the probe() method first.

Can you expand on how you reproduce this problem?  The only real way I
see to call ->sriov_configure() is via the sysfs entry point, and I
would think user-space code would typically not touch that until after
it knows the driver has claimed a device.  But I can certainly imagine
targeted test code that could hit this problem.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1653719

On Tue, 30 May 2017 18:07:18 -0500, Bjorn Helgaas wrote:
> On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> > On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:  
> > > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:  
> > > > PCI core sets the driver pointer before calling ->probe() and only
> > > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > > callback will happily race with probe() and remove(), most likely
> > > > leading to BUGs, since drivers don't expect this.    
> > > 
> > > I guess you're referring to the pci_dev->driver pointer set by
> > > local_pci_probe(), and this is important because sriov_numvfs_store()
> > > checks that pointer, right?  
> > 
> > Yes, exactly.  I initially thought this is how the safety of sriov
> > callback may have been ensured, but since the order of
> > local_pci_probe() and the assignment is what it is, it can't.  
> 
> Right.  I was hoping other subsystems would establish a convention
> about whether we set the ->driver pointer before or after calling the
> driver probe() method, but if there is one, I don't see it.
> local_pci_probe() and really_probe() set ->driver first, but
> pnp_device_probe() calls the probe() method first.

I didn't dig into reordering the pointer setting, to be honest.  I
thought establishing that driver callbacks should generally hold device
lock, whenever possible, would be even better than pointer setting
conventions.

If we order the assignments better, wouldn't we still need appropriate
memory barriers to rely on the order? (:

> Can you expand on how you reproduce this problem?  The only real way I
> see to call ->sriov_configure() is via the sysfs entry point, and I
> would think user-space code would typically not touch that until after
> it knows the driver has claimed a device.  But I can certainly imagine
> targeted test code that could hit this problem.

Correct.  It's not something that users should be triggering often in
normal use.  I also found it by code inspection rather than by getting
an oops.

OTOH if the driver performs FW load or other time-consuming operations
in ->probe() the time window when this can be triggered may be counted
in seconds.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web