Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Dirk Hohndel 
On Mon, Oct 29, 2007 at 03:53:14AM -0400, Jeff Garzik wrote:

  You would also want to kfree(hidinput) on failure too.

Oops, of course. Thanks for catching that. Here's the updated patch

/D


[INPUT] hidinput_connect incorrectly ignored return value from 
input_register_device

Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

---
 drivers/hid/hid-input.c |   12 ++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..880161b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1186,13 +1186,21 @@ int hidinput_connect(struct hid_device *hid)
 * UGCI) cram a lot of unrelated inputs into the
 * same interface. */
hidinput-report = report;
-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input)) {
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+   return -1;
+   }
hidinput = NULL;
}
}
 
if (hidinput)
-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input)) {
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+   return -1;
+   }
 
return 0;
 }
-- 
gitgui.0.8.4.g8d863

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Jeff Garzik 

Dirk Hohndel wrote:

[INPUT] hidinput_connect incorrectly ignored return value from 
input_register_device

Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

---
 drivers/hid/hid-input.c |   12 ++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..880161b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1186,13 +1186,21 @@ int hidinput_connect(struct hid_device *hid)
 * UGCI) cram a lot of unrelated inputs into the
 * same interface. */
hidinput-report = report;
-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input)) {
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+   return -1;
+   }
hidinput = NULL;
}
}
 
 	if (hidinput)

-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input)) {
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+   return -1;


ACK, thanks for revising

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Jiri Kosina 
On Mon, 29 Oct 2007, Dirk Hohndel wrote:

 [INPUT] hidinput_connect incorrectly ignored return value from 
 input_register_device
 Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

Will apply, thanks a lot.

-- 
Jiri Kosina

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Jeff Garzik 

Dmitry Torokhov wrote:

On 10/29/07, Jiri Kosina [EMAIL PROTECTED] wrote:

On Mon, 29 Oct 2007, Dirk Hohndel wrote:


[INPUT] hidinput_connect incorrectly ignored return value from
input_register_device
Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

Will apply


Please don't - the fix is completely broken for multi-input devices -
if 2nd device fails to register we bail out of hidinput_connect and
thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
never call hidinput_disconnect and who knows what will happen after
that.

hidinput_connect() should properly unwind already registered devices
after failure.


Then the existing code to handle hidinput and input_dev allocation 
failure probably also wants fixing...  Dirk's patch was largely 
following the same logic.


Jeff

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Dirk Hohndel 
On Mon, Oct 29, 2007 at 10:49:03AM -0400, Jeff Garzik wrote:
  Dmitry Torokhov wrote:
  On 10/29/07, Jiri Kosina [EMAIL PROTECTED] wrote:
  On Mon, 29 Oct 2007, Dirk Hohndel wrote:
 
  [INPUT] hidinput_connect incorrectly ignored return value from
  input_register_device
  Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]
  Will apply
  Please don't - the fix is completely broken for multi-input devices -
  if 2nd device fails to register we bail out of hidinput_connect and
  thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
  never call hidinput_disconnect and who knows what will happen after
  that.
  hidinput_connect() should properly unwind already registered devices
  after failure.
 
  Then the existing code to handle hidinput and input_dev allocation failure 
  probably also wants fixing...  Dirk's patch was largely following the same 
  logic.

I was wondering about that. If I didn't get lost in the structures again, I
think it isn't too hard to simply call out directly to hidinput_disconnect to
do the cleanup / unwind; the hid-inputs should contain those devices that
have successfully been registered before we failed. 

Actually, the more I look at the code that bails when it runs out of memory,
the more I wonder about that.

hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
kfree(hidinput);
input_free_device(input_dev);

This either passes a NULL pointer to kfree or to input_free_device. That's
not nice.

Would something like this work?

[PATCH] hidinput_connect ignores retval from input_register_device

Signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

---
 drivers/hid/hid-input.c |   24 ++--
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..5bff5cc 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1149,10 +1149,12 @@ int hidinput_connect(struct hid_device *hid)
hidinput = kzalloc(sizeof(*hidinput), 
GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
-   kfree(hidinput);
-   input_free_device(input_dev);
+   if (hidinput)
+   kfree(hidinput);
+   if (input_dev)
+   input_free_device(input_dev);
err_hid(Out of memory during hid input 
probe);
-   return -1;
+   goto out_unwind;
}
 
input_set_drvdata(input_dev, hid);
@@ -1186,15 +1188,25 @@ int hidinput_connect(struct hid_device *hid)
 * UGCI) cram a lot of unrelated inputs into the
 * same interface. */
hidinput-report = report;
-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input))
+   goto out_cleanup;
hidinput = NULL;
}
}
 
-   if (hidinput)
-   input_register_device(hidinput-input);
+   if (hidinput  input_register_device(hidinput-input))
+   goto out_cleanup;
 
return 0;
+
+out_cleanup:
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+out_unwind:
+   /* unwind the ones we already registered */
+   hidinput_disconnect(hid);
+
+   return -1;
 }
 EXPORT_SYMBOL_GPL(hidinput_connect);
 
-- 
gitgui.0.8.4.g8d863

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Hohndel, Dirk 


On Oct 29, 2007, at 8:33 AM, Dmitry Torokhov wrote:


On 10/29/07, Dirk Hohndel [EMAIL PROTECTED] wrote:


Actually, the more I look at the code that bails when it runs out  
of memory,

the more I wonder about that.

   hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
   input_dev = input_allocate_device();
   if (!hidinput || !input_dev) {
   kfree(hidinput);
   input_free_device(input_dev);

This either passes a NULL pointer to kfree or to  
input_free_device. That's

not nice.



No, that's allright. input_free_device() takes after kfree() so that
it safe to call it with NULL pointer.


Ahh - should have checked that.

Will the rest of the patch work with the call to hidinput_disconnect?  
If yes then
I'll send a cleaned up version that removes the unnecessary checks in  
the case here...


/D

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Jiri Kosina 
On Mon, 29 Oct 2007, Dirk Hohndel wrote:

   hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
   input_dev = input_allocate_device();
   if (!hidinput || !input_dev) {
   kfree(hidinput);
   input_free_device(input_dev);
 This either passes a NULL pointer to kfree or to input_free_device. That's
 not nice.

Actually both of the cases are fine -- it is valid to pass NULL pointer to 
kfree() and to input_free_device() too.

 Would something like this work?

Yes, I think that this patch is in principle fine, modulo the redundant 
NULL-ptr checks.

Thanks,

-- 
Jiri Kosina

Re: [PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

2007-10-29 Thread Dirk Hohndel 
[sorry - mail config screwup caused this to bounce for the more restrictive
of the recipient addresses, including the mailing lists, so resending again]

On Mon, Oct 29, 2007 at 06:28:36PM +0100, Jiri Kosina wrote:

 
   
   

  Would something like this work? 
  
   
   
   

 Yes, I think that this patch is in principle fine, modulo the redundant   
   

 NULL-ptr checks.  
   



 
Thanks everyone with helping me to get this right! Which I hope this one is :-) 

 


 
/D  

 


[PATCH] hidinput_connect ignores retval from input_register_device

signed-off-by: Dirk Hohndel [EMAIL PROTECTED]

---
 drivers/hid/hid-input.c |   18 ++
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..aa9b52d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1152,7 +1152,7 @@ int hidinput_connect(struct hid_device *hid)
kfree(hidinput);
input_free_device(input_dev);
err_hid(Out of memory during hid input 
probe);
-   return -1;
+   goto out_unwind;
}
 
input_set_drvdata(input_dev, hid);
@@ -1186,15 +1186,25 @@ int hidinput_connect(struct hid_device *hid)
 * UGCI) cram a lot of unrelated inputs into the
 * same interface. */
hidinput-report = report;
-   input_register_device(hidinput-input);
+   if (input_register_device(hidinput-input))
+   goto out_cleanup;
hidinput = NULL;
}
}
 
-   if (hidinput)
-   input_register_device(hidinput-input);
+   if (hidinput  input_register_device(hidinput-input))
+   goto out_cleanup;
 
return 0;
+
+out_cleanup:
+   input_free_device(hidinput-input);
+   kfree(hidinput);
+out_unwind:
+   /* unwind the ones we already registered */
+   hidinput_disconnect(hid);
+
+   return -1;
 }
 EXPORT_SYMBOL_GPL(hidinput_connect);
 
-- 
gitgui.0.8.4.g8d863