Re: [RFC] PCI device list locking - take 2

[Chris Wright]

* Greg KH ([e-mail]) wrote:
>  static void *pci_seq_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct list_head *p = &pci_devices;
> +	struct pci_dev *dev = NULL;
>  	loff_t n = *pos;
>  
> -	/* XXX: surely we need some locking for traversing the list? */
> +	dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
>  	while (n--) {
> -		p = p->next;
> -		if (p == &pci_devices)
> -			return NULL;
> +		dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
> +		if (dev == NULL)
> +			goto exit;

I think this still has the same problem.  pci_get_device grabs lock,
walks list, gets ref, and drops lock.  But the ref doesn't hold it on the
list, right?.  So some pci_remove_* could do list_del(&dev->global_list),
poison the prev/next pointers.  Subsequent pci_get_device would do ->next
and oops.  It seems the lock needs to be held for entire start/next/stop
sequence, or the ref needs to keep it on list.

> +struct pci_dev * 
> +pci_get_subsys(unsigned int vendor, unsigned int device,
<snip>
> +exit:
> +	if (from)
> +		pci_put_dev(from);
> +	if (dev)
> +		pci_get_dev(dev);

Heh, the hch in me notes that pci_{put,get}_dev already check NULL device ;-)

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net