Skip to contentSkip to conversation

PCI device list locking

RFC In review Greg KH 3 participants 4 messages Jun 17, 2003 PCI

lore
Share & tools
lore

Greg KH

author
#0 lore raw

RFC 1/1 PCI device list locking

Hi all,

Below is a patch against 2.5.72 that adds a pci_bus_lock to protect the pci device and pci bus lists. This is one of the final steps in finally getting proper locking for these lists (which previously had none.) I've been stressing it pretty hard on some pci hotplug boxes I have here, with no problems, but I wanted to get everyone's opinion first, before sending it off to Linus.

I know pci_find_bus() isn't properly protected right now, but the number of callers of this function are pretty small (only pci hotplug drivers), so the risk is quite low.

Comments? Places I missed protecting?

thanks,

greg k-h

# PCI: add locking for when we touch the pci device lists.

drivers/pci/bus.c +6 -0
diff -Nru a/drivers/pci/bus.c b/drivers/pci/bus.c
@@ -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 +11 -0
diff -Nru a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c
@@ -187,8 +187,12 @@
 	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);
 	return 0;

@@ -237,14 +241,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
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
@@ -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 +17 -4
diff -Nru a/drivers/pci/proc.c b/drivers/pci/proc.c
@@ -12,6 +12,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/smp_lock.h>
+#include "pci.h"
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>

@@ -311,20 +312,32 @@
 	struct list_head *p = &pci_devices;
 	loff_t n = *pos;
 
-	/* XXX: surely we need some locking for traversing the list? */
+	spin_lock(&pci_bus_lock);
 	while (n--) {
 		p = p->next;
-		if (p == &pci_devices)
-			return NULL;
+		if (p == &pci_devices) {
+			p = NULL;
+			goto exit;
+		}
 	}
+exit:
+	spin_unlock(&pci_bus_lock);
 	return p;
 }
+
 static void *pci_seq_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct list_head *p = v;
+	void *ret = NULL;
+
 	(*pos)++;
-	return p->next != &pci_devices ? (void *)p->next : NULL;
+	spin_lock(&pci_bus_lock);
+	if (p->next != &pci_devices)
+	       ret = (void *)p->next;
+	spin_unlock(&pci_bus_lock);
+	return ret;
 }
+
 static void pci_seq_stop(struct seq_file *m, void *v)
 {
 	/* release whatever locks we need */
drivers/pci/search.c +39 -13

105 lines not on this page · raw message

#1 lore raw
Quoted 2 lines · * Greg KH ([email protected]) wrote:

Comments? Places I missed protecting?

Is it safe to ignore pcibios_init? This happens after smp_init, but are could there be multiple events (that would effect pcibios_sort)?

Quoted 11 lines
drivers/pci/proc.c +1 -0
@@ -12,6 +12,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/smp_lock.h>
+#include "pci.h"
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>

drivers/pci/proc.c line 315

Quoted 5 lines
drivers/pci/proc.c +1 -1
@@ -311,20 +312,32 @@
 	struct list_head *p = &pci_devices;
 	loff_t n = *pos;
 
-	/* XXX: surely we need some locking for traversing the list? */
+	spin_lock(&pci_bus_lock);

should you just grab this lock here (pci_seq_start), and release in pci_seq_stop, holding for duration of ->seq_start() ->seq_next() ->seq_stop(). IOW, what happens when you grab list element in ->seq_start(), it's removed from list, you reference a bogus ->next pointer in ->seq_next()?

thanks,
-chris

Signature · 1 line
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
author
#2 lore raw
Quoted 6 lines · Chris Wright
Quoted 2 lines · * Greg KH ([email protected]) wrote:

Comments? Places I missed protecting?

Is it safe to ignore pcibios_init? This happens after smp_init, but are could there be multiple events (that would effect pcibios_sort)?

Yes, I'm ignoring the PCI startup code for now. That's a twisty maze of horrible passages that I'm going to try to tackle after this step...

Quoted 14 lines
Quoted 14 lines
drivers/pci/proc.c +1 -0
@@ -12,6 +12,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/smp_lock.h>
+#include "pci.h"
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>

@@ -311,20 +312,32 @@
 	struct list_head *p = &pci_devices;
 	loff_t n = *pos;
Quoted 8 lines

drivers/pci/proc.c line 315

Quoted 2 lines
drivers/pci/proc.c +1 -1
-	/* XXX: surely we need some locking for traversing the list? */
+	spin_lock(&pci_bus_lock);

should you just grab this lock here (pci_seq_start), and release in pci_seq_stop, holding for duration of ->seq_start() ->seq_next() ->seq_stop(). IOW, what happens when you grab list element in ->seq_start(), it's removed from list, you reference a bogus ->next pointer in ->seq_next()?

Hm, good point. Let me go check to see if we invalidate a ->next pointer when we remove the device...

Ugh, we don't. And what's even worse is that data could be gone...

I'll work on this one a bit...

thanks for taking a look at this.

greg k-h

#3 lore raw
In reply to Greg KH

Review

Quoted 3 lines · Greg KH
(quoted diff) +1 -0
+		spin_lock(&pci_bus_lock);

I have a vague feeling that some code somewhere may be calling these functions from interrupt context.

I may have misremembered, but perhaps a sprinkling of WARN_ON(irqs_disabled()) would be prudent for now.