[PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

[PATCH] INPUT: fix hidinput_connect ignoring retval from input_register_device

[Dirk Hohndel]

[INPUT] hidinput_connect incorrectly ignored return value from input_register_device

Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com>

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..c8640e7 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1186,13 +1186,19 @@ 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);
+					return -1;
+				}
 				hidinput = NULL;
 			}
 		}
 
 	if (hidinput)
-		input_register_device(hidinput->input);
+		if (input_register_device(hidinput->input)) {
+			input_free_device(hidinput->input);
+			return -1;
+		}
 
 	return 0;
 }
-- 
gitgui.0.8.4.g8d863

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

[Jeff Garzik]

Dirk Hohndel wrote:
> [INPUT] hidinput_connect incorrectly ignored return value from input_register_device
> 
> Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com>
> 
> ---
>  drivers/hid/hid-input.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index dd332f2..c8640e7 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1186,13 +1186,19 @@ 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);
> +					return -1;
> +				}
>  				hidinput = NULL;
>  			}
>  		}
>  
>  	if (hidinput)
> -		input_register_device(hidinput->input);
> +		if (input_register_device(hidinput->input)) {
> +			input_free_device(hidinput->input);
> +			return -1;
> +		}

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

Thanks for attacking this newly added warning!

	Jeff

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

[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 <hohndel@linux.intel.com>

---
 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

[Jeff Garzik]

Dirk Hohndel wrote:
> [INPUT] hidinput_connect incorrectly ignored return value from input_register_device
> 
> Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com>
> 
> ---
>  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

[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 <hohndel@linux.intel.com>

Will apply, thanks a lot.

-- 
Jiri Kosina

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

[Dmitry Torokhov]

On 10/29/07, Jiri Kosina <jikos@jikos.cz> 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 <hohndel@linux.intel.com>
>
> 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.

-- 
Dmitry

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

[Jiri Kosina]

On Mon, 29 Oct 2007, Dmitry Torokhov wrote:

> 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.

You are of course right, stupid me. I won't commit anything before first 
morning coffee any more.

Thanks a lot.

-- 
Jiri Kosina

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

[Jeff Garzik]

Dmitry Torokhov wrote:
> On 10/29/07, Jiri Kosina <jikos@jikos.cz> 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 <hohndel@linux.intel.com>
>> 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

[Dirk Hohndel]

On Mon, Oct 29, 2007 at 10:49:03AM -0400, Jeff Garzik wrote:
>  Dmitry Torokhov wrote:
> > On 10/29/07, Jiri Kosina <jikos@jikos.cz> 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 <hohndel@linux.intel.com>
> >> 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 <hohndel@linux.intel.com>

---
 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

[Dmitry Torokhov]

On 10/29/07, Dirk Hohndel <hohndel@linux.intel.com> 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.

-- 
Dmitry

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

[Hohndel, Dirk]

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

> On 10/29/07, Dirk Hohndel <hohndel@linux.intel.com> 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

[Dmitry Torokhov]

On 10/29/07, Hohndel, Dirk <hohndel@linux.intel.com> wrote:
>
> On Oct 29, 2007, at 8:33 AM, Dmitry Torokhov wrote:
>
> > On 10/29/07, Dirk Hohndel <hohndel@linux.intel.com> 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?

Yes, I think it will.

-- 
Dmitry

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

[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

[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 <hohndel@linux.intel.com>

---
 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