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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
