Hi Jiri,
This is mail is in regard to your commit for hidaw devices. We were seeing kernel crashes while connect and disconnect a hidraw device and we were hitting
rb tree corruption in vmalloc and kernel was crashing because of null pointer de-reference.
Later we figure out that was because the hid device disconnect is freeing memory which later got access by hidraw_read and hid_write.
commit df0cfd6990347c20ae031f3f34137cba274f1972
Author: Jiri Kosina <[email protected]>
Date: Thu Nov 1 11:33:26 2012 +0100
HID: hidraw: put old deallocation mechanism in place
This basically reverts commit 4fe9f8e203fda. It causes multiple problems,
namely:
We found following commit by Ratan Nalumasu was helping in solving our issue
commit 4fe9f8e203fdad1524c04beb390f3c6099781ed9
Author: Ratan Nalumasu <[email protected]>
Date: Sat Sep 22 11:46:30 2012 -0700
HID: hidraw: don't deallocate memory when it is in use
When a device is unplugged, wait for all processes that have opened the device
to close before deallocating the device.
But there was a bug in Ratan's that commit which was causing slab memory corruption. We fixed that bug and now our issue solved. I can give evidence on issue.
The bug that was there in the commit that he was deleting list after words and feeing head of it before.
I am attaching the patch.
-regards
Manoj
-----------------------------------------------------------------------
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index a745163..612a655 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -113,7 +113,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
__u8 *buf;
int ret = 0;
- if (!hidraw_table[minor]) {
+ if (!hidraw_table[minor] || !hidraw_table[minor]->exist) {
ret = -ENODEV;
goto out;
}
@@ -261,7 +261,7 @@ static int hidraw_open(struct inode *inode, struct file *file)
}
mutex_lock(&minors_lock);
- if (!hidraw_table[minor]) {
+ if (!hidraw_table[minor] || !hidraw_table[minor]->exist) {
err = -ENODEV;
goto out_unlock;
}
@@ -302,39 +302,38 @@ static int hidraw_fasync(int fd, struct file *file, int on)
return fasync_helper(fd, file, on, &list->fasync);
}
+static void drop_ref(struct hidraw *hidraw, int exists_bit)
+{
+ if (exists_bit) {
+ hid_hw_close(hidraw->hid);
+ hidraw->exist = 0;
+ if (hidraw->open)
+ wake_up_interruptible(&hidraw->wait);
+ } else {
+ --hidraw->open;
+ }
+
+ if (!hidraw->open && !hidraw->exist) {
+ device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
+ hidraw_table[hidraw->minor] = NULL;
+ kfree(hidraw);
+ }
+}
+
static int hidraw_release(struct inode * inode, struct file * file)
{
unsigned int minor = iminor(inode);
- struct hidraw *dev;
struct hidraw_list *list = file->private_data;
- int ret;
- int i;
mutex_lock(&minors_lock);
- if (!hidraw_table[minor]) {
- ret = -ENODEV;
- goto unlock;
- }
list_del(&list->node);
- dev = hidraw_table[minor];
- if (!--dev->open) {
- if (list->hidraw->exist) {
- hid_hw_power(dev->hid, PM_HINT_NORMAL);
- hid_hw_close(dev->hid);
- } else {
- kfree(list->hidraw);
- }
- }
-
- for (i = 0; i < HIDRAW_BUFFER_SIZE; ++i)
- kfree(list->buffer[i].value);
kfree(list);
- ret = 0;
-unlock:
- mutex_unlock(&minors_lock);
- return ret;
+ drop_ref(hidraw_table[minor], 0);
+
+ mutex_unlock(&minors_lock);
+ return 0;
}
static long hidraw_ioctl(struct file *file, unsigned int cmd,
@@ -539,18 +538,9 @@ void hidraw_disconnect(struct hid_device *hid)
struct hidraw *hidraw = hid->hidraw;
mutex_lock(&minors_lock);
- hidraw->exist = 0;
-
- device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
- hidraw_table[hidraw->minor] = NULL;
+ drop_ref(hidraw, 1);
- if (hidraw->open) {
- hid_hw_close(hid);
- wake_up_interruptible(&hidraw->wait);
- } else {
- kfree(hidraw);
- }
mutex_unlock(&minors_lock);
}
EXPORT_SYMBOL_GPL(hidraw_disconnect);
--
-Regards
Manoj
On Mon, 22 Jul 2013, Manoj Chourasia wrote:
> This is mail is in regard to your commit for hidaw devices. We were
> seeing kernel crashes while connect and disconnect a hidraw device and
> we were hitting rb tree corruption in vmalloc and kernel was crashing
> because of null pointer de-reference. Later we figure out that was
> because the hid device disconnect is freeing memory which later got
> access by hidraw_read and hid_write.
Now applied, thanks Manoj, thanks Peter.
--
Jiri Kosina
SUSE Labs
On Friday 09 August 2013 11:31:15 Jiri Kosina wrote:
> On Mon, 22 Jul 2013, Manoj Chourasia wrote:
> > This is mail is in regard to your commit for hidaw devices. We were
> > seeing kernel crashes while connect and disconnect a hidraw device and
> > we were hitting rb tree corruption in vmalloc and kernel was crashing
> > because of null pointer de-reference. Later we figure out that was
> > because the hid device disconnect is freeing memory which later got
> > access by hidraw_read and hid_write.
>
> Now applied, thanks Manoj, thanks Peter.
I just noticed an issue with this patch, after playing with some receivers I
saw 8 hidraw device nodes. Every USB receiver creates two hidraw nodes, one
for the receiver and one for the paired device.
UPower seems to keep the file descriptor open which prevents the hidraw device
from being removed. Shouldn't the implementation of hidraw be changed to
separate open file descriptors from the actual HID devices? That should allow
/dev/hidrawX devices to get removed whenever a HID device is gone.
Regards,
Peter
Hi Jiri,
What is the final proposed fix for this issue?
Thanks
-Manoj
-----Original Message-----
From: Peter Wu [mailto:[email protected]]
Sent: Tuesday, August 20, 2013 7:44 PM
To: Jiri Kosina
Cc: Manoj Chourasia; [email protected]
Subject: Re: [PATCH] hidraw: correctly deallocate memory on device disconnect
On Friday 09 August 2013 11:31:15 Jiri Kosina wrote:
> On Mon, 22 Jul 2013, Manoj Chourasia wrote:
> > This is mail is in regard to your commit for hidaw devices. We were
> > seeing kernel crashes while connect and disconnect a hidraw device
> > and we were hitting rb tree corruption in vmalloc and kernel was
> > crashing because of null pointer de-reference. Later we figure out
> > that was because the hid device disconnect is freeing memory which
> > later got access by hidraw_read and hid_write.
>
> Now applied, thanks Manoj, thanks Peter.
I just noticed an issue with this patch, after playing with some receivers I saw 8 hidraw device nodes. Every USB receiver creates two hidraw nodes, one for the receiver and one for the paired device.
UPower seems to keep the file descriptor open which prevents the hidraw device from being removed. Shouldn't the implementation of hidraw be changed to separate open file descriptors from the actual HID devices? That should allow /dev/hidrawX devices to get removed whenever a HID device is gone.
Regards,
Peter