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 <younghua.zheng@xxxxxxxxx>
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 <younghua.zheng@xxxxxxxxx>
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;
};
--
1.7.9.5
Best regards
Yonghua
On Fri, Aug 09, 2013 at 04:45:19PM +0800, yonghua zheng wrote:
> Hi Jiri,
>
> We met another kernel panic issue in hidraw_report_event while using
> BT remote on our Android system, the panic log below shows a unhandle
> kernel paging request at virtual address 001000f0, this can happen if
> hidraw_report_event access a hidraw_list node which is already removed
> by list_del() in hidraw_release.
>
> One last thing list_del() does is set entry->next = LIST_POISON1;
> where LIST_POISON1 is 00100100, and this should be able to explain why
> panic address is 001000f0, hidraw_report_event try to access somewhere
> in struct hidraw_list:
>
> [ 1337.530941] Unable to handle kernel paging request at virtual
> address 001000f0
> [ 1337.538679] pgd = a0570000
> [ 1337.541877] [001000f0] *pgd=00000000
> [ 1337.545884] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> [ 1337.551373] Modules linked in: mbt8787 fusion(O) gal3d amp_core(O)
> [ 1337.557834] CPU: 0 Tainted: G O (3.4.50+ #1)
> [ 1337.563512] PC is at hidraw_report_event+0x38/0x98
> [ 1337.568462] LR is at hidraw_report_event+0x70/0x98
> [ 1337.573412] pc : [<8035286c>] lr : [<803528a4>] psr: a0000013
> [ 1337.573416] sp : a0eede30 ip : a0eede30 fp : a0eede54
> [ 1337.575086] [amp_driver] [vpp isr] MsgQ full
> [ 1337.589671] r10: a0b7e4c0 r9 : a035b800 r8 : a0bb002c
> [ 1337.595068] r7 : a0b7e4e4 r6 : 00000001 r5 : 0000001f r4 : 000ffef0
> [ 1337.601809] r3 : a36db808 r2 : 00000020 r1 : 0000001f r0 : a0bb002c
> [ 1337.608553] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 1337.615923] Control: 10c5387d Table: 2157004a DAC: 00000015
>
> So the fix is to grab the minors_lock to avoid list_del in the middle
> of hidraw_report_event()
>
> From 725ed6c402f36e34479739a8d6c68f78f2f96d31 Mon Sep 17 00:00:00 2001
> From: Yonghua Zheng <younghua.zheng@xxxxxxxxx>
> Date: Fri, 9 Aug 2013 15:54:59 +0800
> Subject: [PATCH 1/1] HID: hidraw: need to grab minors_lock in
> hidraw_report_event
>
> It is unsafe to call list_for_each_entry to go through each hidraw_list
> node without grabing the minors_lock, because the dev->list can be
> modified if other process calls hidraw_release to list_del itself from
> the same &dev->list, if this happens, it can result in kernel panic
> due to unable handle kernel paging request at invalid virtual address
>
> Signed-off-by: Yonghua Zheng <younghua.zheng@xxxxxxxxx>
> ---
> drivers/hid/hidraw.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 6f1feb2..8b408c4 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -458,6 +458,7 @@ int hidraw_report_event(struct hid_device *hid, u8
> *data, int len)
> struct hidraw_list *list;
> int ret = 0;
>
> + mutex_lock(&minors_lock);
> list_for_each_entry(list, &dev->list, node) {
> int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
>
> @@ -465,6 +466,7 @@ int hidraw_report_event(struct hid_device *hid, u8
> *data, int len)
> continue;
>
> if (!(list->buffer[list->head].value = kmemdup(data, len,
> GFP_ATOMIC))) {
> + mutex_unlock(&minors_lock);
> ret = -ENOMEM;
> break;
> }
> @@ -472,6 +474,7 @@ int hidraw_report_event(struct hid_device *hid, u8
> *data, int len)
> list->head = new_head;
> kill_fasync(&list->fasync, SIGIO, POLL_IN);
> }
> + mutex_unlock(&minors_lock);
>
> wake_up_interruptible(&dev->wait);
> return ret;
> --
> 1.7.9.5
>
> Best regards
> Yonghua
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
