Skip to contentSkip to conversation

driver core: check 'start' argument in bus iterators

PATCH In review Hannes Reinecke 2 participants 3 messages Apr 16, 2012 Driver core

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 driver core: check 'start' argument in bus iterators

bus_for_each_dev() and bus_find_device() both take a 'start' argument to start the iteration at a specific list entry. However, this list entry might already been detached by the time these functions are called.
So we need to check if the arguments are still valid.

Cc Greg Kroah-Hartmann <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Stable Kernel <[email protected]>
drivers/base/bus.c +6 -0
index 26a06b8..264e498 100644
@@ -297,6 +297,9 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
 	if (!bus)
 		return -EINVAL;
 
+	if (start && !klist_node_attached(&start->p->knode_bus))
+		return -ENODEV;
+
 	klist_iter_init_node(&bus->p->klist_devices, &i,
 			     (start ? &start->p->knode_bus : NULL));
 	while ((dev = next_device(&i)) && !error)
@@ -331,6 +334,9 @@ struct device *bus_find_device(struct bus_type *bus,
 	if (!bus)
 		return NULL;
 
+	if (start && !klist_node_attached(&start->p->knode_bus))
+		return NULL;
+
 	klist_iter_init_node(&bus->p->klist_devices, &i,
 			     (start ? &start->p->knode_bus : NULL));
 	while ((dev = next_device(&i)))
#1 lore raw
Quoted 11 lines · Hannes Reinecke

bus_for_each_dev() and bus_find_device() both take a 'start' argument to start the iteration at a specific list entry. However, this list entry might already been detached by the time these functions are called.
So we need to check if the arguments are still valid.

+       if (start && !klist_node_attached(&start->p->knode_bus))
+               return -ENODEV;
+
       klist_iter_init_node(&bus->p->klist_devices, &i, (start ? &start->p->knode_bus : NULL));

Shouldn't we do this check in klist_iter_init_node()? After a kref_get(), and return -ENODEV from there if the node it as asked to start is gone?

This check otherwise still has the window between the check and the use of it, only very unlikely to hit now, hasn't it?

Kay

#2 lore raw
Quoted 13 lines · Kay Sievers

drivers/base/bus.c int bus_for_each_dev(struct bus_type *bus, struct device *start, · line 304

Quoted 11 lines · Hannes Reinecke

bus_for_each_dev() and bus_find_device() both take a 'start' argument to start the iteration at a specific list entry. However, this list entry might already been detached by the time these functions are called.
So we need to check if the arguments are still valid.

drivers/base/bus.c +3 -0
+       if (start && !klist_node_attached(&start->p->knode_bus))
+               return -ENODEV;
+
       klist_iter_init_node(&bus->p->klist_devices, &i,
                            (start ? &start->p->knode_bus : NULL));
Quoted 4 lines

Shouldn't we do this check in klist_iter_init_node()? After a kref_get(), and return -ENODEV from there if the node it as asked to start is gone?

Sure, if you prefer.

Quoted 3 lines

This check otherwise still has the window between the check and the use of it, only very unlikely to hit now, hasn't it?

yeah, very.
The iteration itself could take some time (locks etc), but the race between those two lines is really tiny.

But yeah, for the sake of correctness, we should.

I'll be sending an updated patch.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)