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

[Konstantin Belousov]

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,