Date: Sun, 24 Nov 2019 18:45:36 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Warner Losh <imp@bsdimp.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r355037 - head/sys/dev/pci Message-ID: <20191124164536.GC2707@kib.kiev.ua> In-Reply-To: <CANCZdfrtKQmGiB=w5Q2x7sTCh4JO63mdJn6fg%2BAXfPfeyQGkfA@mail.gmail.com> References: <201911232343.xANNhqkQ097797@repo.freebsd.org> <20191124131010.GB2707@kib.kiev.ua> <CANCZdfrtKQmGiB=w5Q2x7sTCh4JO63mdJn6fg%2BAXfPfeyQGkfA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Nov 24, 2019 at 07:37:01AM -0700, Warner Losh wrote: > On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov <kostikbel@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191124164536.GC2707>