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.
> }
>
> /* 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,
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
