Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better

On Tue, 20 Sep 2011, James Hogan wrote:

quoted

quoted

Signed-off-by: James Hogan <redacted>
---
 drivers/hid/hidraw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index c79578b..a8c2b7b 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c

@@ -510,13 +510,12 @@ 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));

This does not destroy any open file descriptor and we haven't
registered any kind of hook so hidraw_destroy() will not be called
here.
This seems safe to me.
We also do not check for hidraw->exist on *_open() callback so
including this in the critical section seems fine.

That's good then. Thanks for checking it.

quoted

quoted

-       mutex_lock(&minors_lock);
       hidraw_table[hidraw->minor] = NULL;
-       mutex_unlock(&minors_lock);

       if (hidraw->open) {
               hid_hw_close(hid);

@@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
       } else {
               kfree(hidraw);
       }
+       mutex_unlock(&minors_lock);
 }
 EXPORT_SYMBOL_GPL(hidraw_disconnect);

--

1.7.2.3

Nice catch. I've tested it on linux-next tree and I can confirm the
bug. The fix seems ok to me.

Good, thanks. I have now applied it with your Tested-by:, please shout if 
you don't want that.

Shall I resend without RFC? I'll add Cc: [off-list ref] too if
there aren't objections.

stable@kernel.org wouldn't work anyway due to kernel.org being down ...

-- 
Jiri Kosina
SUSE Labs