PCI device list locking - take 2

RFC In review Greg KH 2 participants 6 messages Jun 18, 2003 – Jun 19, 2003 PCI

lore

Share & tools

lore

Greg KH

author

2003-06-18 21:15 UTC #0 lore raw

RFC 1/1 PCI device list locking - take 2

Hi,

Thanks to Chris Wright and Andrew Morton's comments on my last patch to add locking to the pci device lists, I've redone it. Below is a patch against 2.5.72 that should address everyone's previous concerns.

It adds a lock (pci_bus_lock) that is grabbed whenever the pci lists are accessed, and two new functions, pci_get_device() and pci_get_subsys(). These functions work just like pci_find_device() and pci_find_subsys() did, but they handle the proper reference counting of the pci devices found and passed to them. Then, the drivers/pci/proc.c code was converted to use these functions instead of accessing the pci lists directly.

Again, I'm ignoring the pci startup code, as that is a big rats nest that is next on my list to look at.

Comments on this version?

thanks,

greg k-h

drivers/pci/bus.c +6 -0

@@ -93,7 +93,11 @@
 			continue;
 
 		device_add(&dev->dev);
+
+		spin_lock(&pci_bus_lock);
 		list_add_tail(&dev->global_list, &pci_devices);
+		spin_unlock(&pci_bus_lock);
+
 		pci_proc_attach_device(dev);
 		pci_create_sysfs_dev_files(dev);

@@ -108,7 +112,9 @@
 		 * it and then scan for unattached PCI devices.
 		 */
 		if (dev->subordinate && list_empty(&dev->subordinate->node)) {
+			spin_lock(&pci_bus_lock);
 			list_add_tail(&dev->subordinate->node, &dev->bus->children);
+			spin_unlock(&pci_bus_lock);
 			pci_bus_add_devices(dev->subordinate);
 		}
 	}

drivers/pci/hotplug.c +12 -0

@@ -187,10 +187,15 @@
 	if (pci_dev_driver(dev))
 		return -EBUSY;
 	device_unregister(&dev->dev);
+
+	spin_lock(&pci_bus_lock);
 	list_del(&dev->bus_list);
 	list_del(&dev->global_list);
+	spin_unlock(&pci_bus_lock);
+
 	pci_free_resources(dev);
 	pci_proc_detach_device(dev);
+	pci_put_dev(dev);
 	return 0;
 }
 EXPORT_SYMBOL(pci_remove_device_safe);

@@ -237,14 +242,21 @@
 		pci_remove_behind_bridge(dev);
 		pci_proc_detach_bus(b);
 
+		spin_lock(&pci_bus_lock);
 		list_del(&b->node);
+		spin_unlock(&pci_bus_lock);
+
 		kfree(b);
 		dev->subordinate = NULL;
 	}
 
 	device_unregister(&dev->dev);
+
+	spin_lock(&pci_bus_lock);
 	list_del(&dev->bus_list);
 	list_del(&dev->global_list);
+	spin_unlock(&pci_bus_lock);
+
 	pci_free_resources(dev);
 	pci_proc_detach_device(dev);
 	pci_put_dev(dev);

drivers/pci/pci.h +3 -0

@@ -59,3 +59,6 @@
 extern int pci_visit_dev(struct pci_visit *fn,
 			 struct pci_dev_wrapped *wrapped_dev,
 			 struct pci_bus_wrapped *wrapped_parent);
+
+/* Lock for read/write access to pci device and bus lists */
+extern spinlock_t pci_bus_lock;

drivers/pci/proc.c +25 ~~-19~~

89 lines not on this page · raw message

drivers/pci/search.c +132 ~~-13~~

216 lines not on this page · raw message

include/linux/pci.h +11 -0

@@ -566,6 +566,10 @@
 int pci_find_capability (struct pci_dev *dev, int cap);
 struct pci_bus * pci_find_next_bus(const struct pci_bus *from);
 
+struct pci_dev *pci_get_device (unsigned int vendor, unsigned int device, struct pci_dev *from);
+struct pci_dev *pci_get_subsys (unsigned int vendor, unsigned int device,
+				unsigned int ss_vendor, unsigned int ss_device,
+				struct pci_dev *from);
 int pci_bus_read_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 *val);
 int pci_bus_read_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 *val);
 int pci_bus_read_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 *val);

@@ -686,6 +690,13 @@
 
 static inline struct pci_dev *pci_find_subsys(unsigned int vendor, unsigned int device,
 unsigned int ss_vendor, unsigned int ss_device, const struct pci_dev *from)
+{ return NULL; }
+
+static inline struct pci_dev *pci_get_device (unsigned int vendor, unsigned int device, struct pci_dev *from)
+{ return NULL; }
+
+static inline struct pci_dev *pci_get_subsys (unsigned int vendor, unsigned int device,
+unsigned int ss_vendor, unsigned int ss_device, struct pci_dev *from)
 { return NULL; }
 
 static inline void pci_set_master(struct pci_dev *dev) { }

Chris Wright

2003-06-18 22:20 UTC #1 lore raw

Quoted 5 lines · * Greg KH ([email protected]) wrote:

static void *pci_seq_start(struct seq_file *m, loff_t *pos) {
drivers/pci/proc.c +1 -1
-	struct list_head *p = &pci_devices;
+	struct pci_dev *dev = NULL;
 	loff_t n = *pos;

drivers/pci/proc.c line 319

Quoted 10 lines

drivers/pci/proc.c +4 -4

 
-	/* 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.

Quoted 2 lines

drivers/pci/search.c +2 -0

+struct pci_dev * 
+pci_get_subsys(unsigned int vendor, unsigned int device,

<snip>

drivers/pci/search.c line 211

Quoted 5 lines

drivers/pci/search.c +5 -0

+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

Signature · 1 line

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Greg KH

author

2003-06-18 22:32 UTC #2 lore raw

Quoted 7 lines · Chris Wright

Quoted 6 lines · * Greg KH ([email protected]) wrote:
static void *pci_seq_start(struct seq_file *m, loff_t *pos) {
drivers/pci/proc.c +1 -1
-	struct list_head *p = &pci_devices;
+	struct pci_dev *dev = NULL;
 	loff_t n = *pos;

Quoted 16 lines

drivers/pci/proc.c line 319
Quoted 9 lines
drivers/pci/proc.c +4 -4
-	/* 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.

Hm, I think we should probably just check in pci_get_device() to verify that ->next is really valid. If it isn't just return NULL, as we have no idea what the next device would possibly be. The worse thing that would happen is the proc file would be a bit shorter than expected. If read again, it would be correct, with the previously referenced device now gone.

I don't want to try to hold a lock over start/next/stop as that would just be asking for trouble :)

Sound acceptable?

Quoted 10 lines

Quoted 2 lines
drivers/pci/search.c +2 -0
+struct pci_dev * 
+pci_get_subsys(unsigned int vendor, unsigned int device,
<snip>
drivers/pci/search.c line 211
Quoted 5 lines
drivers/pci/search.c +5 -0
+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 ;-)

Heh, good point, I'll go fix that :)

thanks for looking at this,

greg k-h

Chris Wright

2003-06-18 23:19 UTC #3 lore raw

Quoted 6 lines · * Greg KH ([email protected]) wrote:

Hm, I think we should probably just check in pci_get_device() to verify that ->next is really valid. If it isn't just return NULL, as we have no idea what the next device would possibly be. The worse thing that would happen is the proc file would be a bit shorter than expected. If read again, it would be correct, with the previously referenced device now gone.

I'm not sure testing a valid ->next makes sense. It could be non-NULL, but poison, or if it was using list_del_init, it would be stuck in loop.

Quoted 2 lines

I don't want to try to hold a lock over start/next/stop as that would just be asking for trouble :)

Heh, I agree, it doesn't feel quite right to acquire lock and release lock in separate functions, but in the case of start/show/next/stop this seems to be the design. Alternative here seems to be keeping thing on list with get and deleting from with put on last ref, but that didn't look so simple.

thanks,
-chris

Signature · 1 line

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Greg KH

author

2003-06-19 00:00 UTC #4 lore raw

Quoted 10 lines · Chris Wright

Quoted 6 lines · * Greg KH ([email protected]) wrote:
Hm, I think we should probably just check in pci_get_device() to verify that ->next is really valid. If it isn't just return NULL, as we have no idea what the next device would possibly be. The worse thing that would happen is the proc file would be a bit shorter than expected. If read again, it would be correct, with the previously referenced device now gone.
I'm not sure testing a valid ->next makes sense. It could be non-NULL, but poison, or if it was using list_del_init, it would be stuck in loop.

When we take the devices off of the list, after list_del(), still under the lock, we can null out the list pointers. Then, later under the lock, we can check the pointer before we move to it. We aren't doing fancy list_* functions with the pci device lists at all.

Quoted 8 lines

Quoted 2 lines
I don't want to try to hold a lock over start/next/stop as that would just be asking for trouble :)
Heh, I agree, it doesn't feel quite right to acquire lock and release lock in separate functions, but in the case of start/show/next/stop this seems to be the design. Alternative here seems to be keeping thing on list with get and deleting from with put on last ref, but that didn't look so simple.

No, that isn't the proper model anyway. After the device is removed, we don't want to still have it floating around on the device lists, even if someone has a reference to it. That would make things just too difficult :)

I'll go and try to see how the above proposed changes will work out, and see if I can see any side affects when beating on /proc/pci on a pci hotplug box...

thanks,

greg k-h

Chris Wright

2003-06-19 00:08 UTC #5 lore raw

Quoted 8 lines · * Greg KH ([email protected]) wrote:

Quoted 2 lines · Chris Wright
I'm not sure testing a valid ->next makes sense. It could be non-NULL, but poison, or if it was using list_del_init, it would be stuck in loop.
When we take the devices off of the list, after list_del(), still under the lock, we can null out the list pointers. Then, later under the lock, we can check the pointer before we move to it. We aren't doing fancy list_* functions with the pci device lists at all.

Ah, ok, that should work.

thanks,
-chris

Signature · 1 line

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net