multiple interface test case

Roman Kagan rkagan at mail.ru
Wed Apr 13 00:41:22 EDT 2005


On Tue, Apr 12, 2005 at 11:56:22PM +0400, Roman Kagan wrote:
> On Tue, Apr 12, 2005 at 11:08:27PM +0400, Roman Kagan wrote:
> > On Tue, Apr 12, 2005 at 10:08:56PM +0400, Roman Kagan wrote:
> > > in drivers/base/bus.c:
> > > 
> > > void device_release_driver(struct device * dev)
> > > {
> > >         struct device_driver * drv = dev->driver;
> > >         if (drv) {
> > >                 sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
> > >                 sysfs_remove_link(&dev->kobj, "driver");
> > >                 list_del_init(&dev->driver_list);
> > >                 device_detach_shutdown(dev);
> > >                 if (drv->remove)
> > >                         drv->remove(dev);
> > >                 dev->driver = NULL;
> > >         }
> > > }
> > > 
> > > static void driver_detach(struct device_driver * drv)
> > > {
> > >         struct list_head * entry, * next;
> > >         list_for_each_safe(entry, next, &drv->devices) {
> > >                 struct device * dev = container_of(entry, struct device, driver_list);
> > >                 device_release_driver(dev);
> > >         }
> > > }
> > > [...]
> > > yet.  Can't it be that list_for_each_safe() is not that safe against
> > > removal of the entries inside the loop body?
> > 
> > Hmm, looks fishy to me:
> > 
> > #define list_for_each_safe(pos, n, head) \
> >         for (pos = (head)->next, n = pos->next; pos != (head); \
> >                 pos = n, n = pos->next)
> > 
> > So if entries are deleted inside the loop body (with list_del_init() in
> > our case), the loop is screwed up?
> 
> To make my point clear: list_for_each_safe() seems to be safe only
> w.r.t. the deletion of the current entry.  However, in our
> multi-interface scenario, we start with device_release_driver() for the
> first interface on the drv->devices list, and then (through
> drv->remove(), which calls disconnect(), which calls
> usb_driver_release_interface() for other interfaces) we call
> device_release_driver() for other interfaces and empty the drv->devices
> list, while list_for_each_safe() has already cached the (now invalid)
> next entry, and thus goes into an infinite loop.

Seems to be exactly the same problem with klists:

drivers/base/dd.c:

static int __remove_driver(struct device * dev, void * unused)
{
	device_release_driver(dev);
	return 0;
}

void driver_detach(struct device_driver * drv)
{
	driver_for_each_device(drv, NULL, NULL, __remove_driver);
}

drivers/base/driver.c:

int driver_for_each_device(struct device_driver * drv, struct device * start, 
			   void * data, int (*fn)(struct device *, void *))
{
	struct klist_iter i;
	struct device * dev;
	int error = 0;

	if (!drv)
		return -EINVAL;

	klist_iter_init_node(&drv->klist_devices, &i,
			     start ? &start->knode_driver : NULL);
	while ((dev = next_device(&i)) && !error)
		error = fn(dev, data);
	klist_iter_exit(&i);
	return error;
}

i.e. the list iteration advances to the next entry, without expecting it
to be invalidated inside the loop body (invalidation of the current
entry is expected).  Since the invalidation of an entry consists of
setting prev and next to point to itself, the loop runs forever.

Roman.



More information about the Usbatm mailing list