[1/1] HID: hidraw: Add spinlock in struct hidraw to protect list

Commit Message

yonghua zheng Aug. 15, 2013, 2:57 p.m. UTC

Retitle

Hi Jiri,

As hidraw_report_event can be called from interrupt context, it is a mistake
to use mutex_lock for protecting the list member in my previous patch, so
update the patch which adds a spinlock in struct hidraw to protect the list
member from concurrent access:

From 21e3dfdf124065dac8bb56f4a7ed48d20870323b Mon Sep 17 00:00:00 2001
From: Yonghua Zheng <[e-mail]>
Date: Wed, 14 Aug 2013 17:43:36 +0800
Subject: [PATCH 1/1] HID: hidraw: Add spinlock in struct hidraw to protect
 list

It is unsafe to call list_for_each_entry in hidraw_report_event to
traverse each hidraw_list node without a lock protection, the list
could be modified if someone calls hidraw_release and list_del to
remove itself from the list, this can cause hidraw_report_event
to touch a deleted list struct and panic.

To prevent this, introduce a spinlock in struct hidraw to protect
list from concurrent access.

Signed-off-by: Yonghua Zheng <[e-mail]>

Comments

Jiri Kosina Aug. 26, 2013, 12:03 p.m. UTC | #1

On Thu, 15 Aug 2013, Yonghua Zheng wrote:

> As hidraw_report_event can be called from interrupt context, it is a mistake
> to use mutex_lock for protecting the list member in my previous patch, so
> update the patch which adds a spinlock in struct hidraw to protect the list
> member from concurrent access:

Hi,

thanks for the patch.

I already have

	commit 212a871a3934beccf43431608c27ed2e05a476ec
	Author: Manoj Chourasia <[e-mail]>
	Date:   Mon Jul 22 15:33:13 2013 +0530

	    HID: hidraw: correctly deallocate memory on device disconnect

in the tree, which collides with your patch. Could you please check what 
changes are needed on top of it so that it makes sense for my 'for-next' 
branch, rebase, and resend to me?

Thanks,

Patch

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 6f1feb2..a5372b8 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -253,6 +253,7 @@  static int hidraw_open(struct inode *inode, struct file *file)
 	unsigned int minor = iminor(inode);
 	struct hidraw *dev;
 	struct hidraw_list *list;
+	unsigned long flags;
 	int err = 0;
 
 	if (!(list = kzalloc(sizeof(struct hidraw_list), GFP_KERNEL))) {
@@ -266,11 +267,6 @@  static int hidraw_open(struct inode *inode, struct file *file)
 		goto out_unlock;
 	}
 
-	list->hidraw = hidraw_table[minor];
-	mutex_init(&list->read_mutex);
-	list_add_tail(&list->node, &hidraw_table[minor]->list);
-	file->private_data = list;
-
 	dev = hidraw_table[minor];
 	if (!dev->open++) {
 		err = hid_hw_power(dev->hid, PM_HINT_FULLON);
@@ -283,9 +279,16 @@  static int hidraw_open(struct inode *inode, struct file *file)
 		if (err < 0) {
 			hid_hw_power(dev->hid, PM_HINT_NORMAL);
 			dev->open--;
+			goto out_unlock;
 		}
 	}
 
+	list->hidraw = hidraw_table[minor];
+	mutex_init(&list->read_mutex);
+	spin_lock_irqsave(&hidraw_table[minor]->list_lock, flags);
+	list_add_tail(&list->node, &hidraw_table[minor]->list);
+	spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags);
+	file->private_data = list;
 out_unlock:
 	mutex_unlock(&minors_lock);
 out:
@@ -309,6 +312,7 @@  static int hidraw_release(struct inode * inode, struct file * file)
 	struct hidraw_list *list = file->private_data;
 	int ret;
 	int i;
+	unsigned long flags;
 
 	mutex_lock(&minors_lock);
 	if (!hidraw_table[minor]) {
@@ -316,8 +320,10 @@  static int hidraw_release(struct inode * inode, struct file * file)
 		goto unlock;
 	}
 
-	list_del(&list->node);
 	dev = hidraw_table[minor];
+	spin_lock_irqsave(&dev->list_lock, flags);
+	list_del(&list->node);
+	spin_unlock_irqrestore(&dev->list_lock, flags);
 	if (!--dev->open) {
 		if (list->hidraw->exist) {
 			hid_hw_power(dev->hid, PM_HINT_NORMAL);
@@ -457,7 +463,9 @@  int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
 	struct hidraw *dev = hid->hidraw;
 	struct hidraw_list *list;
 	int ret = 0;
+	unsigned long flags;
 
+	spin_lock_irqsave(&dev->list_lock, flags);
 	list_for_each_entry(list, &dev->list, node) {
 		int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
 
@@ -468,10 +476,12 @@  int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
 			ret = -ENOMEM;
 			break;
 		}
+
 		list->buffer[list->head].len = len;
 		list->head = new_head;
 		kill_fasync(&list->fasync, SIGIO, POLL_IN);
 	}
+	spin_unlock_irqrestore(&dev->list_lock, flags);
 
 	wake_up_interruptible(&dev->wait);
 	return ret;
@@ -519,6 +529,7 @@  int hidraw_connect(struct hid_device *hid)
 	}
 
 	init_waitqueue_head(&dev->wait);
+	spin_lock_init(&dev->list_lock);
 	INIT_LIST_HEAD(&dev->list);
 
 	dev->hid = hid;
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index 2451662..ddf5261 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -23,6 +23,7 @@  struct hidraw {
 	wait_queue_head_t wait;
 	struct hid_device *hid;
 	struct device *dev;
+	spinlock_t list_lock;
 	struct list_head list;
 };