Re: [PATCH] hidraw: fix list->buffer race condition
From: Benjamin Tissoires <hidden>
Date: 2016-10-05 07:53:34
Hi Gary, On Oct 04 2016 or thereabouts, Gary King wrote:
hidraw input events are stored for each file descriptor in a lockless circular queue. no memory barriers were used when the queue was updated, which caused intermittent kernel panics due to heap corruption when used on multi-core ARM systems. add memory barriers to ensure that value updates are observable before the head and tail referents are updated.
As a foreword, I must confess I am not that comfortable with memory barriers on SMP. I have a hard time trying to understand where the code can be reordered and why you are having the heap corruption and how these barriers solve the issue.
Change-Id: Ifb50f5ebe13c55c83aa105c5cd5926ca16fd93e0 Signed-off-by: Gary King <redacted> Reviewed-on: http://prn-ocugerrit01.thefacebook.com:8080/88
This doesn't look like a public URL, please drop if not.
quoted hunk ↗ jump to hunk
Reviewed-by: Ahmed Amin <redacted> --- drivers/hid/hidraw.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c index f0e2757..dc3465f 100644 --- a/drivers/hid/hidraw.c +++ b/drivers/hid/hidraw.c@@ -53,6 +53,7 @@ static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, mutex_lock(&list->read_mutex); while (ret == 0) { + smp_rmb(); if (list->head == list->tail) { add_wait_queue(&list->hidraw->wait, &wait); set_current_state(TASK_INTERRUPTIBLE);@@ -98,7 +99,9 @@ static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, kfree(list->buffer[list->tail].value); list->buffer[list->tail].value = NULL; + smp_wmb(); list->tail = (list->tail + 1) & (HIDRAW_BUFFER_SIZE - 1); + smp_wmb();
How does these barriers be needed? To me, list->tail gets accessed just before, so I doubt the compiler would decide to reorder the code without changing the semantic. Again, I am not an expert regarding memory barriers, but either you convince me that this is the best solution, either there is an other solution (like protecting the circular buffer with spinlocks between the feeder and consumer).
quoted hunk ↗ jump to hunk
} out: mutex_unlock(&list->read_mutex);@@ -487,7 +490,7 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len) 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); -
Nitpicking, please do not drop this empty line, see the kernel coding style, an empty line is required after a declaration.
quoted hunk ↗ jump to hunk
+ smp_rmb(); if (new_head == list->tail) continue;@@ -496,7 +499,9 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len) break; } list->buffer[list->head].len = len; + smp_wmb(); list->head = new_head; + smp_wmb();
Same comment than before, I don't understand how the barrier can help you here.
kill_fasync(&list->fasync, SIGIO, POLL_IN); } spin_unlock_irqrestore(&dev->list_lock, flags); -- 1.9.1
Cheers, Benjamin