hotplug fixes

This is a bunch of changes to the hotplug code, keventd, PCI, netdevices and call_usermodehelper().

It should fix a number of the PCMCIA problems people have been reporting. There are still some questions and issues over this stuff - I'll cover that in a separate email.

- We have been getting deadlocks at hotplug time because call_usermodehelper is synchronous: it returns control after the usermode application has exited. But in some places (esp. [un]register_netdevice) this happens with a lock held (rtnl_lock). This cause `ifconfig' to get stuck.

So call_usermodehelper has been made asynchronous. Linus did some wizardry with CLONE_VFORK to make this happen safely.

- PCMCIA layer calls call_usermodehelper from within keventd. But call_usermodehelper blocks until keventd has run the helper! Duh.

This patch special-cases the situation where keventd is calling call_usermodehelper().

- Add a check for an empty executable path string in call_usermodehelper. (This check can probably be removed from the USB code now?)

- Previously, the netdevice layer was calling call_usermodehelper from the netdevice notifier callchain. This is a worry because:

- The code which traverses notifier call chains is not safe wrt changes in the chain. Calling out to userland applications increases the risk that the chain will be changed under our feet.

- The sweet little loop at the end of unregister_netdevice can cause /sbin/hotplug to be called four times per second for thirty seconds while the kernel is waiting for orphaned IPV4 fragments to expire.

So the patch takes the hotplug call off the notifier chain and open codes the calls.

- There is a ghastly race with all netdevices. Hundreds of 'em. The typical netdevice probe function is:

xxx_probe()

  {
	dev = init_etherdev();	/* Calls register_netdevice */
	...
	futz around for tens of milliseconds
	...
	dev->open = xxx_open;
	/* Device is ready for open */

}

xxx_open()
{
does stuff with hardware
}

The netdevice becomes eligible for an open as soon as it is registered. That open will fail because

a) The hardware isn't ready yet and

b) The netdevice.open field hasn't been initialised yet.

Presumably this race hasn't been noticed before because:

a) Opening the device usually occurs a "long" time after the probe. Well, it used to. But now we're running the `open' from within init_etherdev(), via /sbin/hotplug.

b) Modular drivers are accidentally protected by the BKL. The probing runs under sys_insert_module() which takes the BKL. The netdevice open goes through sys_ioctl() which takes the BKL.

But if the driver sleeps in its probe routine (eg: cs89x0 does it explicitly) then we lose. On SMP or UP.

The right fix is to take the register_netdevice call out of init_etherdev() and change the drivers so they don't register their netdevice until it's ready to be opened.

Not being in the mood to edit 178 files, the approach I took in this patch is to introduce a new system-wide semaphore `dev_probe_lock()' which is taken around the PCI probe. The netdevice ioctl layer also claims this lock so the race is fixed.

Presumably other parts of the kernel which do device probing must also protect themselves by calling dev_probe_[un]lock(). This is
important: running /sbin/hotplug from within the kernel is going to change timings quite a lot.

- There's an obscure bug in tty_io.c:release_dev(). I kindly put it there in pre7.

release_dev() needs to ensure that all pending scheduled tasks have been executed before it starts deallocating resources. To cater for this I created the function `run_schedule_tasks()'. This simply runs the queue in the context of the caller.

The problem is, release_dev() is sometimes called from within the do_exit()->__exit_files() path. If there are pending tasks (and there usually aren't) ee end up trying to fork a child from a half-exitted parent and the child oopses accessing current->files.

So I renamed run_schedule_tasks() to flush_scheduled_tasks() and it now correctly runs the scheduled tasks within keventd's context.

Incidentally, all users of schedule_task() need to be reviewed for races on the close()/release() path. I think they can end up shutting down their hardware and deallocating their resources while there are scheduled tasks outstanding.

The fix is to call flush_scheduled_tasks() as soon as the subsystem is in a state where new scheduled_tasks() cannot occur (eg: the interrupts have been disabled).

Note: flush_scheduled_tasks() has the same problem as del_timer_sync(): you mustn't call it when you're holding locks which would prevent the scheduled task to complete:

xxx_intr()

  {
	schedule_task(xxx_bh);

}

xxx_bh()

  {
	down(&some_sem);	/* Or spin_lock() */
	...

}

xxx_close()

  {
	down(&some_sem);
	flush_scheduled_tasks();	/* Deadlock */

}

- kerneldocified a few functions.

@@ -150,8 +150,9 @@
 asmlinkage void schedule(void);
 
 extern int schedule_task(struct tq_struct *task);
-extern void run_schedule_tasks(void);
+extern void flush_scheduled_tasks(void);
 extern int start_context_thread(void);
+extern int current_is_keventd(void);
 
 /*
  * The default fd array needs to be at least BITS_PER_LONG,

@@ -63,6 +63,8 @@
 extern int get_option(char **str, int *pint);
 extern char *get_options(char *str, int nints, int *ints);
 extern unsigned long memparse(char *ptr, char **retptr);
+extern void dev_probe_lock(void);
+extern void dev_probe_unlock(void);
 
 extern int session_of_pgrp(int pgrp);
 

@@ -300,18 +300,25 @@
 pci_announce_device(struct pci_driver *drv, struct pci_dev *dev)
 {
 	const struct pci_device_id *id;
+	int ret = 0;
 
 	if (drv->id_table) {
 		id = pci_match_device(drv->id_table, dev);
-		if (!id)
-			return 0;
+		if (!id) {
+			ret = 0;
+			goto out;
+		}
 	} else
 		id = NULL;
+
+	dev_probe_lock();
 	if (drv->probe(dev, id) >= 0) {
 		dev->driver = drv;
-		return 1;
+		ret = 1;
 	}
-	return 0;
+	dev_probe_unlock();
+out:
+	return ret;
 }
 
 int

@@ -360,9 +367,9 @@
 	if (!hotplug_path[0])
 		return;
 
-	sprintf(class_id, "PCI_CLASS=%X", pdev->class);
-	sprintf(id, "PCI_ID=%X/%X", pdev->vendor, pdev->device);
-	sprintf(sub_id, "PCI_SUBSYS_ID=%X/%X", pdev->subsystem_vendor, pdev->subsystem_device);
+	sprintf(class_id, "PCI_CLASS=%04X", pdev->class);
+	sprintf(id, "PCI_ID=%04X:%04X", pdev->vendor, pdev->device);
+	sprintf(sub_id, "PCI_SUBSYS_ID=%04X:%04X", pdev->subsystem_vendor, pdev->subsystem_device);
 	sprintf(bus_id, "PCI_SLOT_NAME=%s", pdev->slot_name);
 
 	i = 0;

@@ -302,9 +302,9 @@
 {
 	struct mm_struct * mm = tsk->mm;
 
+	mm_release();
 	if (mm) {
 		atomic_inc(&mm->mm_count);
-		mm_release();
 		if (mm != tsk->active_mm) BUG();
 		/* more a memory barrier than a real lock */
 		task_lock(tsk);

@@ -519,6 +519,7 @@
 		return -EINVAL;
 	}
 
+	dev_probe_lock();
 	rtnl_lock();
 
 	if ((dev = __dev_get_by_name(ifr.ifr_name)) == NULL) {

@@ -649,10 +650,12 @@
 	}
 done:
 	rtnl_unlock();
+	dev_probe_unlock();
 	return ret;
 
 rarok:
 	rtnl_unlock();
+	dev_probe_unlock();
 	if (copy_to_user(arg, &ifr, sizeof(struct ifreq)))
 		return -EFAULT;
 	return 0;

@@ -716,14 +716,6 @@
 	/* Mount the root filesystem.. */
 	mount_root();
 
-#if defined(CONFIG_HOTPLUG) && defined(CONFIG_NET)
-	/* do this after other 'do this last' stuff, because we want
-	 * to minimize spurious executions of /sbin/hotplug
-	 * during boot-up
-	 */
-	net_notifier_init();
-#endif
-
 	mount_devfs_fs ();
 
 #ifdef CONFIG_BLK_DEV_INITRD

@@ -1262,7 +1262,7 @@
 	 * Make sure that the tty's task queue isn't activated. 
 	 */
 	run_task_queue(&tq_timer);
-	run_schedule_tasks();
+	flush_scheduled_tasks();
 
 	/* 
 	 * The release_mem function takes care of the details of clearing

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

+ if (current_is_keventd()) {
+ /* We can't wait on keventd! */
+ __call_usermodehelper(&sub_info);
+ } else {
+ schedule_task(&tqs);
+ down(&sem); /* Wait until keventd has started the subprocess */
+ }

That's sick. Do we have to? The PCMCIA coded obviously wants an async call_usermodehelper() or it wouldn't have been using schedule_task() for it in the first place, would it? Can't we pass an 'int async' arg to call_usermodehelper() instead of doing it this way?

dwmw2




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

All user-mode-helpers are async as of this patch.

The reason for the serialization is that we need to wait until the exec() has taken place - so that the arguments that the caller set up haven't disappeared from the caller stack. The actual execution is asynchronous anyway.

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

OK, I see what's happening. But is it strictly necessary to special-case it? If we can't use schedule_task() in all situations and hence have to have code for doing it ourself - why not just do it ourself all the time, keeping the code simpler?

--
dwmw2

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Well, call_usermodehelper here _is_ async. We're just waiting for acknowledgement that the subprocess has safely taken copies of our local data (argv, envp, etc). And waiting to pick up the return value from execve().

We can't call schedule_task() and then just return from call_usermodehelper(), unless we take kmalloced copies of everything (including the tq_struct) and then free them
after the vfork.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

The /sbin/hotplug script I sent around last week avoids this by doing its work in the background.

I see the underlying problem here as being that network hotplug hooks are called at the wrong time ... because they are piggybacking onto existing APIs, which weren't designed for hotplug support. Different aspects of that root cause were all over your list of problems (seeming to trigger all the nastiest ones :-).

To my way of thought (I don't claim expertise in Linux networking!) the _clean_ fix here is accept that the network driver API needs an update for hotplugging. Not a big one, I suspect. Likely we can start with a "ready to open this device!" call, which hotplug-ready network drivers call at the end of their PCI or USB probe routines.

We've needed such changes in both PCI and USB driver APIs ... suggesting that such changes are to be expected as new subsystems start to learn how to autoconfigure themselves.

Sure.

- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/