Fix locking in mousedev

Fix locking in mousedev

[Pete Zaitcev]

If a process is closing /dev/input/mice and an mouse disconnects simulta-
neously, the system is likely to oops. This usually happens when someone
hits <Alt><Ctrl>F1 or logs out from X, and flips a KVM while the system
is reacting.

I reproduced the issue by running this:
  while true; do cat </dev/null >/dev/input/mice; done
This way, it oopses on 2nd or 3rd disconnect reliably. With the patch,
I can disconnect the mouse 20 times.

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---

Discussion

One of the race scenarios is related to the list of handles. The cat
calls mousedev_close -> mixdev_release, does list_for_each to walk for
all handles for a given handler. Iterations are longish while it does
input_close_device -> hidinput_close -> usbhid_close -> usb_kill_urb,
which sleeps briefly. Into this gap goes khubd and does hid_disconnect ->
hidinput_disconnect -> input_unregister_device. This corrupts the list
of handles which cat process is walking.

I was unable to devise a scheme to protect the stock h_list adequately,
so I implemented a private list of mousedev instances, which can be
protected correctly.

Dmitry, please consider getting rid of the list of handles entirely.
The other major user is drivers/char/keyboard.c.

Other than that, the patch is straightforward. It adds a static mutex
to guard common data structures. It has to be static because instances
of mousedev share common structures, such as the mousedev_table[].

This should be uncontroversial, but please let me know if I missed
something obvious.

-- Pete

diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 664bcc8..2425c2a 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/major.h>
 #include <linux/device.h>
@@ -64,6 +65,7 @@ struct mousedev {
 	char name[16];
 	wait_queue_head_t wait;
 	struct list_head list;
+	struct list_head h_node;
 	struct input_handle handle;
 
 	struct mousedev_hw_data packet;
@@ -108,10 +110,13 @@ static unsigned char mousedev_imps_seq[] = { 0xf3, 200, 0xf3, 100, 0xf3, 80 };
 static unsigned char mousedev_imex_seq[] = { 0xf3, 200, 0xf3, 200, 0xf3, 80 };
 
 static struct input_handler mousedev_handler;
+static LIST_HEAD(mousedev_h_list);
 
 static struct mousedev *mousedev_table[MOUSEDEV_MINORS];
 static struct mousedev mousedev_mix;
 
+static DEFINE_MUTEX(mousedev_lock);
+
 #define fx(i)  (mousedev->old_x[(mousedev->pkt_count - (i)) & 03])
 #define fy(i)  (mousedev->old_y[(mousedev->pkt_count - (i)) & 03])
 
@@ -366,11 +371,9 @@ static void mousedev_free(struct mousedev *mousedev)
 
 static void mixdev_release(void)
 {
-	struct input_handle *handle;
-
-	list_for_each_entry(handle, &mousedev_handler.h_list, h_node) {
-		struct mousedev *mousedev = handle->private;
+	struct mousedev *mousedev;
 
+	list_for_each_entry(mousedev, &mousedev_h_list, h_node) {
 		if (!mousedev->open) {
 			if (mousedev->exist)
 				input_close_device(&mousedev->handle);
@@ -386,6 +389,7 @@ static int mousedev_release(struct inode * inode, struct file * file)
 
 	mousedev_fasync(-1, file, 0);
 
+	mutex_lock(&mousedev_lock);
 	list_del(&list->node);
 
 	if (!--list->mousedev->open) {
@@ -398,6 +402,7 @@ static int mousedev_release(struct inode * inode, struct file * file)
 				mousedev_free(list->mousedev);
 		}
 	}
+	mutex_unlock(&mousedev_lock);
 
 	kfree(list);
 	return 0;
@@ -406,7 +411,6 @@ static int mousedev_release(struct inode * inode, struct file * file)
 static int mousedev_open(struct inode * inode, struct file * file)
 {
 	struct mousedev_list *list;
-	struct input_handle *handle;
 	struct mousedev *mousedev;
 	int i;
 
@@ -417,11 +421,16 @@ static int mousedev_open(struct inode * inode, struct file * file)
 #endif
 		i = iminor(inode) - MOUSEDEV_MINOR_BASE;
 
-	if (i >= MOUSEDEV_MINORS || !mousedev_table[i])
+	mutex_lock(&mousedev_lock);
+	if (i >= MOUSEDEV_MINORS || !mousedev_table[i]) {
+		mutex_unlock(&mousedev_lock);
 		return -ENODEV;
+	}
 
-	if (!(list = kzalloc(sizeof(struct mousedev_list), GFP_KERNEL)))
+	if (!(list = kzalloc(sizeof(struct mousedev_list), GFP_KERNEL))) {
+		mutex_unlock(&mousedev_lock);
 		return -ENOMEM;
+	}
 
 	spin_lock_init(&list->packet_lock);
 	list->pos_x = xres / 2;
@@ -432,16 +441,16 @@ static int mousedev_open(struct inode * inode, struct file * file)
 
 	if (!list->mousedev->open++) {
 		if (list->mousedev->minor == MOUSEDEV_MIX) {
-			list_for_each_entry(handle, &mousedev_handler.h_list, h_node) {
-				mousedev = handle->private;
+			list_for_each_entry(mousedev, &mousedev_h_list, h_node) {
 				if (!mousedev->open && mousedev->exist)
-					input_open_device(handle);
+					input_open_device(&mousedev->handle);
 			}
 		} else
 			if (!mousedev_mix.open && list->mousedev->exist)
 				input_open_device(&list->mousedev->handle);
 	}
 
+	mutex_unlock(&mousedev_lock);
 	return 0;
 }
 
@@ -604,7 +613,6 @@ static ssize_t mousedev_read(struct file * file, char __user * buffer, size_t co
 	return count;
 }
 
-/* No kernel lock - fine */
 static unsigned int mousedev_poll(struct file *file, poll_table *wait)
 {
 	struct mousedev_list *list = file->private_data;
@@ -631,14 +639,19 @@ static struct input_handle *mousedev_connect(struct input_handler *handler, stru
 	struct class_device *cdev;
 	int minor = 0;
 
+	mutex_lock(&mousedev_lock);
+
 	for (minor = 0; minor < MOUSEDEV_MINORS && mousedev_table[minor]; minor++);
 	if (minor == MOUSEDEV_MINORS) {
+		mutex_unlock(&mousedev_lock);
 		printk(KERN_ERR "mousedev: no more free mousedev devices\n");
 		return NULL;
 	}
 
-	if (!(mousedev = kzalloc(sizeof(struct mousedev), GFP_KERNEL)))
+	if (!(mousedev = kzalloc(sizeof(struct mousedev), GFP_KERNEL))) {
+		mutex_unlock(&mousedev_lock);
 		return NULL;
+	}
 
 	INIT_LIST_HEAD(&mousedev->list);
 	init_waitqueue_head(&mousedev->wait);
@@ -664,6 +677,9 @@ static struct input_handle *mousedev_connect(struct input_handler *handler, stru
 	sysfs_create_link(&input_class.subsys.kset.kobj, &cdev->kobj,
 			  mousedev->name);
 
+	list_add_tail(&mousedev->h_node, &mousedev_h_list);
+
+	mutex_unlock(&mousedev_lock);
 	return &mousedev->handle;
 }
 
@@ -672,6 +688,9 @@ static void mousedev_disconnect(struct input_handle *handle)
 	struct mousedev *mousedev = handle->private;
 	struct mousedev_list *list;
 
+	mutex_lock(&mousedev_lock);
+	list_del(&mousedev->h_node);
+
 	sysfs_remove_link(&input_class.subsys.kset.kobj, mousedev->name);
 	class_device_destroy(&input_class,
 			MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + mousedev->minor));
@@ -687,6 +706,7 @@ static void mousedev_disconnect(struct input_handle *handle)
 			input_close_device(handle);
 		mousedev_free(mousedev);
 	}
+	mutex_unlock(&mousedev_lock);
 }
 
 static const struct input_device_id mousedev_ids[] = {

Re: Fix locking in mousedev

[Dmitry Torokhov]

Hi Pete,

On 2/28/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> If a process is closing /dev/input/mice and an mouse disconnects simulta-
> neously, the system is likely to oops. This usually happens when someone
> hits <Alt><Ctrl>F1 or logs out from X, and flips a KVM while the system
> is reacting.
>
> I reproduced the issue by running this:
>  while true; do cat </dev/null >/dev/input/mice; done
> This way, it oopses on 2nd or 3rd disconnect reliably. With the patch,
> I can disconnect the mouse 20 times.
>
> Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
>
> ---
>
> Discussion
>
> One of the race scenarios is related to the list of handles. The cat
> calls mousedev_close -> mixdev_release, does list_for_each to walk for
> all handles for a given handler. Iterations are longish while it does
> input_close_device -> hidinput_close -> usbhid_close -> usb_kill_urb,
> which sleeps briefly. Into this gap goes khubd and does hid_disconnect ->
> hidinput_disconnect -> input_unregister_device. This corrupts the list
> of handles which cat process is walking.
>
> I was unable to devise a scheme to protect the stock h_list adequately,
> so I implemented a private list of mousedev instances, which can be
> protected correctly.
>
> Dmitry, please consider getting rid of the list of handles entirely.
> The other major user is drivers/char/keyboard.c.
>

I agree that handlers should not access h_list nad use their own
private lists instead. However input core still needs that list to
maintain its books.

> Other than that, the patch is straightforward. It adds a static mutex
> to guard common data structures. It has to be static because instances
> of mousedev share common structures, such as the mousedev_table[].
>
> This should be uncontroversial, but please let me know if I missed
> something obvious.
>

I agree with the patch, unfortunately in lands squarely in the middle
of me restructuring the code (swiitch to struct device, proper
refcounting, etc) but I will try to adopt it.

-- 
Dmitry

Re: Fix locking in mousedev

[Pete Zaitcev]

On Fri, 9 Mar 2007 09:28:49 -0500, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
> On 2/28/07, Pete Zaitcev <zaitcev@redhat.com> wrote:

> > Dmitry, please consider getting rid of the list of handles entirely.
> > The other major user is drivers/char/keyboard.c.
> 
> I agree that handlers should not access h_list nad use their own
> private lists instead. However input core still needs that list to
> maintain its books.

Hmm. So it appears that my patch wastes 2 ulongs per input handle
then, in the name of preventing oopses. OK.

> I agree with the patch, unfortunately in lands squarely in the middle
> of me restructuring the code (swiitch to struct device, proper
> refcounting, etc) but I will try to adopt it.

Don't worry about this too much, I can always retest and resubmit,
as long as the main idea is agreeable to you. My users are on RHEL.
Therefore, my main concern is to get something into your tree in
order to avoid this regressing in the future. Meanwhile, users of
our 2.6.9 and 2.6.18 forks get a version of this patch.

-- Pete

Re: Fix locking in mousedev

[Dmitry Torokhov]

On 3/9/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> On Fri, 9 Mar 2007 09:28:49 -0500, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
> > On 2/28/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
>
> > > Dmitry, please consider getting rid of the list of handles entirely.
> > > The other major user is drivers/char/keyboard.c.
> >
> > I agree that handlers should not access h_list nad use their own
> > private lists instead. However input core still needs that list to
> > maintain its books.
>
> Hmm. So it appears that my patch wastes 2 ulongs per input handle
> then, in the name of preventing oopses. OK.
>

Think about the time when everyone switches to evdev-based input
drivers. Then we can forget about mousedev wasting some extra memory
;)

-- 
Dmitry