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

[Warner Losh]

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

Warner

>  }
> >
> >  /* Find a device_t by vendor/device ID */
> >
> > Modified: head/sys/dev/pci/pci_user.c
> >
> ==============================================================================
> > --- head/sys/dev/pci/pci_user.c       Sat Nov 23 23:41:21 2019
> (r355036)
> > +++ head/sys/dev/pci/pci_user.c       Sat Nov 23 23:43:52 2019
> (r355037)
> > @@ -119,7 +119,7 @@ static d_ioctl_t  pci_ioctl;
> >
> >  struct cdevsw pcicdev = {
> >       .d_version =    D_VERSION,
> > -     .d_flags =      D_NEEDGIANT,
> > +     .d_flags =      0,
> >       .d_open =       pci_open,
> >       .d_close =      pci_close,
> >       .d_ioctl =      pci_ioctl,
>