svn commit: r355037 - head/sys/dev/pci

[Konstantin Belousov]

On Sun, Nov 24, 2019 at 07:37:01AM -0700, Warner Losh wrote:
> On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov <kostikbel at gmail.com>
> wrote:
> 
> > On Sat, Nov 23, 2019 at 11:43:52PM +0000, Warner Losh wrote:
> > > Author: imp
> > > Date: Sat Nov 23 23:43:52 2019
> > > New Revision: 355037
> > > URL: https://svnweb.freebsd.org/changeset/base/355037
> > >
> > > Log:
> > >   Push Giant down one layer
> > >
> > >   The /dev/pci device doesn't need GIANT, per se. However, one routine
> > >   that it calls, pci_find_dbsf implicitly does. It walks a list that can
> > >   change when PCI scans a new bus. With hotplug, this means we could
> > >   have a race with that scanning. To prevent that, take out Giant around
> > >   scanning the list.
> > >
> > >   However, given that we have places in the tree that drop giant, if
> > >   held when we call into them, the whole use of Giant to protect newbus
> > >   may be less effective that we desire, so add a comment about why we're
> > >   talking it out, and we'll address the issue when we lock newbus with
> > >   something other than Giant.
> > >
> > > Modified:
> > >   head/sys/dev/pci/pci.c
> > >   head/sys/dev/pci/pci_user.c
> > >
> > > Modified: head/sys/dev/pci/pci.c
> > >
> > ==============================================================================
> > > --- head/sys/dev/pci/pci.c    Sat Nov 23 23:41:21 2019        (r355036)
> > > +++ head/sys/dev/pci/pci.c    Sat Nov 23 23:43:52 2019        (r355037)
> > > @@ -445,18 +445,21 @@ pci_find_bsf(uint8_t bus, uint8_t slot, uint8_t
> > func)
> > >  device_t
> > >  pci_find_dbsf(uint32_t domain, uint8_t bus, uint8_t slot, uint8_t func)
> > >  {
> > > -     struct pci_devinfo *dinfo;
> > > +     struct pci_devinfo *dinfo = NULL;
> > >
> > > +     /* Giant because newbus is Giant locked revisit with newbus
> > locking */
> > > +     mtx_lock(&Giant);
> > >       STAILQ_FOREACH(dinfo, &pci_devq, pci_links) {
> > >               if ((dinfo->cfg.domain == domain) &&
> > >                   (dinfo->cfg.bus == bus) &&
> > >                   (dinfo->cfg.slot == slot) &&
> > >                   (dinfo->cfg.func == func)) {
> > > -                     return (dinfo->cfg.dev);
> > > +                     break;
> > >               }
> > >       }
> > > +     mtx_unlock(&Giant);
> > >
> > > -     return (NULL);
> > > +     return (dinfo != NULL ? dinfo->cfg.dev : NULL);
> > I do not think this change is correct. If the parallel hotplug, or
> > rather, hot-unplug event occurs, then dinfo potentially becomes invalid
> > right after the Giant unlock, which makes both this function and its
> > callers to access freed memory. Having caller to lock a newbus lock
> > around both the call and consumption of the returned data is required.
> 
> 
> There are many data lifetime issues. If anything the PCI user device calls
> drops Giant and then picks it back up again we are in the same boat... I
> totally agree this is a bad situation, but can only really be fixed by
> locking newbus with a different lock than Giant and likely using some kind
> of reference count for device_t that are handed out...
> 
> In the mean time, I'll move giant back up into the ioctl routine and hope
> it isn't dropped by things it calls..m

I think we can start at least by marking the Giant acqusitions that are
related to newbus.

I never saw anybody talking publically why the naive translation of newbus
Giant into a sleepable lock, e.g. sx in exclusive mode, cannot work.  From
my memory, one of the big issues is that many sleeps done at probe/attach,
need to drop the newbus lock.