[patch] hiddev: fix incorrect hiddev freeing

[patch] hiddev: fix incorrect hiddev freeing

[Johannes Weiner]

When hiddev_open() fails for whatever reason, free the just allocated
hiddev_list structure shared hiddev potentially still in use.

The hiddev is freed in device disconnect/last close of the device file
and must not be freed while there are possibly existing references to
it.

This is probably responsible for these

  http://kerneloops.org/oops.php?number=221185
  http://kerneloops.org/oops.php?number=220365

where a reader sleeps on the waitqueue, the device gets disconnected
(exist -> 0) another user tries to open it, fails on the exist check
and frees the hiddev from the table.  The finish_wait() in the reader
will then dereference the hiddev to get to the waitqueue and oopses.

This was introduced by commit 079034073faf974973baa0256b029451f6e768ad
"HID: hiddev cleanup -- handle all error conditions properly".

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Oliver Neukum <oliver@neukum.name>
---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 4940e4d..00ea1ed 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -306,7 +306,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
 	return 0;
 bail:
 	file->private_data = NULL;
-	kfree(list->hiddev);
+	kfree(list);
 	return res;
 }
 

Re: [patch] hiddev: fix incorrect hiddev freeing

[Johannes Weiner]

On Mon, Mar 09, 2009 at 03:31:51AM +0100, Johannes Weiner wrote:
> When hiddev_open() fails for whatever reason, free the just allocated
> hiddev_list structure shared hiddev potentially still in use.
                       ^
                   instead of

Sorry, it's late (or early?)  Need a resend?

> The hiddev is freed in device disconnect/last close of the device file
> and must not be freed while there are possibly existing references to
> it.
> 
> This is probably responsible for these
> 
>   http://kerneloops.org/oops.php?number=221185
>   http://kerneloops.org/oops.php?number=220365
> 
> where a reader sleeps on the waitqueue, the device gets disconnected
> (exist -> 0) another user tries to open it, fails on the exist check
> and frees the hiddev from the table.  The finish_wait() in the reader
> will then dereference the hiddev to get to the waitqueue and oopses.
> 
> This was introduced by commit 079034073faf974973baa0256b029451f6e768ad
> "HID: hiddev cleanup -- handle all error conditions properly".
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Oliver Neukum <oliver@neukum.name>
> ---
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 4940e4d..00ea1ed 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -306,7 +306,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
>  	return 0;
>  bail:
>  	file->private_data = NULL;
> -	kfree(list->hiddev);
> +	kfree(list);
>  	return res;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] hiddev: fix incorrect hiddev freeing

[Johannes Weiner]

On Mon, Mar 09, 2009 at 03:31:51AM +0100, Johannes Weiner wrote:
> When hiddev_open() fails for whatever reason, free the just allocated
> hiddev_list structure shared hiddev potentially still in use.
> 
> The hiddev is freed in device disconnect/last close of the device file
> and must not be freed while there are possibly existing references to
> it.
> 
> This is probably responsible for these
> 
>   http://kerneloops.org/oops.php?number=221185
>   http://kerneloops.org/oops.php?number=220365
> 
> where a reader sleeps on the waitqueue, the device gets disconnected
> (exist -> 0) another user tries to open it, fails on the exist check
> and frees the hiddev from the table.  The finish_wait() in the reader
> will then dereference the hiddev to get to the waitqueue and oopses.
> 
> This was introduced by commit 079034073faf974973baa0256b029451f6e768ad
> "HID: hiddev cleanup -- handle all error conditions properly".
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Oliver Neukum <oliver@neukum.name>
> ---
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 4940e4d..00ea1ed 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -306,7 +306,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
>  	return 0;
>  bail:
>  	file->private_data = NULL;
> -	kfree(list->hiddev);
> +	kfree(list);
>  	return res;

This isn't responsible for the above quoted oopsen but I think I found
the real issue.  Resend coming soon.

Dear stable team, I mixed up the commit date with the authoring date.
The bugs were introduced after .28, so there is no need to backport
anything unless my fixes fail to get into .29.  So please ignore for
now.

	Hannes