[PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Hans de Goede]

Having a dangling pointer in intfdata is no good, and may actually bite
other drivers which rely on frameworks which only call dev_set_drvdata
on the interface's device if no drvdata has been set (which is how I
found out that usb-hid-core leaves the dangling pointer in there).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/usbhid/hid-core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5bf91db..70d760f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1296,6 +1296,7 @@ err_free:
 	kfree(usbhid);
 err:
 	hid_destroy_device(hid);
+	usb_set_intfdata(intf, NULL);
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Jiri Slaby]

On 05/21/2012 09:39 PM, Hans de Goede wrote:
> other drivers which rely on frameworks which only call dev_set_drvdata
> on the interface's device if no drvdata has been set

This looks very broken as it relies on an undocumented behavior. If they
want to do that they should:
* set intfdata to NULL
* call some hook that may set intfdata
* set intfdata to whatever they want if it is still NULL

Right?

What are those frameworks?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/usbhid/hid-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..70d760f 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1296,6 +1296,7 @@ err_free:
>  	kfree(usbhid);
>  err:
>  	hid_destroy_device(hid);
> +	usb_set_intfdata(intf, NULL);
>  	return ret;
>  }
>  
> 
-- 
js
suse labs

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Hans de Goede]

Hi,

On 05/22/2012 09:56 AM, Jiri Slaby wrote:
> On 05/21/2012 09:39 PM, Hans de Goede wrote:
>> other drivers which rely on frameworks which only call dev_set_drvdata
>> on the interface's device if no drvdata has been set
>
> This looks very broken as it relies on an undocumented behavior.

I don't see how expecting intfdata to be NULL when an USB driver's
probe function gets called is broken, esp. since most
USB drivers will unconditionally set intfdata to something from
their probe functions, so it seems reasonable to assume that it is
not pointing to anything before probe gets called.

Also note that on disconnect / forced unbind, the USB core
will set intfdata to NULL. itself, enforcing it to be NULL
for unbound interfaces, except when a probe function
screws up and sets intfdata even though the probe failed.

> If they
> want to do that they should:
> * set intfdata to NULL
> * call some hook that may set intfdata
> * set intfdata to whatever they want if it is still NULL
>
> Right?

Wrong, why would they need to explictly NULL intfdata? it should
be NULL when their probe gets called. I cannot believe we are
even having this discussion, are you really trying to argue
that leaving intfdata as a dangling pointer, rather then setting
it to NULL (*) is better???

*) or moving the setting of it to a point where probe can no longer
fail.

> What are those frameworks?
v4l2-device.c does this, the call sequence is:

-some driver's probe method gets called
-that driver can either call dev_set_drvdata itself, if it has its
  own uses for it, or leave it as is (which should be NULL at this point)
-v4l2_device_register gets called on a v4l2_device struct, with a
  device argument, if that device does not have any drvdata set, it will
  set drvdata to point to the v4l2_device struct.

The purpose of this is to try and bring some standardization to what
drvdata will point to for v4l2 devices. But this whole shebang is not
really relevant.

Leaving around a dangling pointer in any circumstancces is just BAD,
very very BAD.

Regards,

Hans





>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/hid/usbhid/hid-core.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 5bf91db..70d760f 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1296,6 +1296,7 @@ err_free:
>>   	kfree(usbhid);
>>   err:
>>   	hid_destroy_device(hid);
>> +	usb_set_intfdata(intf, NULL);
>>   	return ret;
>>   }
>>
>>

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Jiri Slaby]

On 05/22/2012 10:09 AM, Hans de Goede wrote:
> Hi,
> 
> On 05/22/2012 09:56 AM, Jiri Slaby wrote:
>> On 05/21/2012 09:39 PM, Hans de Goede wrote:
>>> other drivers which rely on frameworks which only call dev_set_drvdata
>>> on the interface's device if no drvdata has been set
>>
>> This looks very broken as it relies on an undocumented behavior.
> 
> I don't see how expecting intfdata to be NULL when an USB driver's
> probe function gets called is broken, esp. since most
> USB drivers will unconditionally set intfdata to something from
> their probe functions, so it seems reasonable to assume that it is
> not pointing to anything before probe gets called.

As you can see, it is not.

>> If they
>> want to do that they should:
>> * set intfdata to NULL
>> * call some hook that may set intfdata
>> * set intfdata to whatever they want if it is still NULL
>>
>> Right?
> 
> Wrong, why would they need to explictly NULL intfdata?

Because they test it against NULL later?

> it should
> be NULL when their probe gets called.

No, this is not documented anywhere as far as I can see. And many
drivers just don't do that. The same for PCI and likely other buses
(like HID).

> I cannot believe we are
> even having this discussion, are you really trying to argue
> that leaving intfdata as a dangling pointer, rather then setting
> it to NULL (*) is better???

No, the patch looks correct. But you are expecting something which is
not still guaranteed.

>> What are those frameworks?
> v4l2-device.c does this, the call sequence is:
> 
> -some driver's probe method gets called
> -that driver can either call dev_set_drvdata itself, if it has its
>  own uses for it, or leave it as is (which should be NULL at this point)

(No, it should not. Change all the drivers or the USB and PCI cores,
document that and then you can expect it.)

> -v4l2_device_register gets called on a v4l2_device struct, with a
>  device argument, if that device does not have any drvdata set, it will
>  set drvdata to point to the v4l2_device struct.

As it stands, v4l now actually depends on its drivers to set intfdata
unconditionally. Either to NULL or some valid pointer...

regards,
-- 
js
suse labs

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Hans de Goede]

Hi,

On 05/22/2012 10:29 AM, Jiri Slaby wrote:
> On 05/22/2012 10:09 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 05/22/2012 09:56 AM, Jiri Slaby wrote:
>>> On 05/21/2012 09:39 PM, Hans de Goede wrote:
>>>> other drivers which rely on frameworks which only call dev_set_drvdata
>>>> on the interface's device if no drvdata has been set
>>>
>>> This looks very broken as it relies on an undocumented behavior.
>>
>> I don't see how expecting intfdata to be NULL when an USB driver's
>> probe function gets called is broken, esp. since most
>> USB drivers will unconditionally set intfdata to something from
>> their probe functions, so it seems reasonable to assume that it is
>> not pointing to anything before probe gets called.
>
> As you can see, it is not.

Notice I used the word "reasoanble" I still believe it is reasonable
to expect intfdata to be NULL. intfdata is just an alias for dev_drvdata,
and if no driver is bound to a device what should its drvdata be?

So we have:
1) A device which does not have a driver bound
2) A potential drivers probe method being called (which will only
    happen if 1. is true)
3) That probe method expecting drvdata to be NULL since no driver is
    bound

Surprise surprise (not). If no driver is bound what else can drvdata
be but NULL, anything else would be a reminisce of a previous driver,
and thus very likely a dangling pointer.

>> it should
>> be NULL when their probe gets called.
>
> No, this is not documented anywhere as far as I can see. And many
> drivers just don't do that. The same for PCI and likely other buses
> (like HID).

Well on driver unbind the USB core explictly sets intfdata to NULL,
which to me clearly signals intent that intfdata should be NULL when
no driver is bound.

The usb code does not do clear intfdata on probe fail, I don't know why,
likely because it expects a failed probe to not set it in the first
place! But it probably is a very good idea to make the USB core set
intfdata to NULL after a failed probe to ensure that it is NULL
when no driver is bound, independent of driver behavior.

>> I cannot believe we are
>> even having this discussion, are you really trying to argue
>> that leaving intfdata as a dangling pointer, rather then setting
>> it to NULL (*) is better???
>
> No, the patch looks correct. But you are expecting something which is
> not still guaranteed.

Well then lets work towards making it guaranteed, since I still believe
the following holds true:
1) drvdata is for a driver to store a pointer to driver specific data
2) If no driver is bound, there is no driver specific data associated with
    the device
3) Thus logically drvdata should be NULL if no driver is bound.

I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
on probe failure.

Regards,

Hans

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Jiri Kosina]

On Tue, 22 May 2012, Hans de Goede wrote:

> Well then lets work towards making it guaranteed, since I still believe
> the following holds true:
> 1) drvdata is for a driver to store a pointer to driver specific data
> 2) If no driver is bound, there is no driver specific data associated with
>    the device
> 3) Thus logically drvdata should be NULL if no driver is bound.
> 
> I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
> on probe failure.

Hans,

I believe this is a good thing per se, but shouldn't this rather be done 
in driver core, to guarantee that this hold across all buses?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Hans de Goede]

Hi,

On 05/23/2012 12:00 AM, Jiri Kosina wrote:
> On Tue, 22 May 2012, Hans de Goede wrote:
>
>> Well then lets work towards making it guaranteed, since I still believe
>> the following holds true:
>> 1) drvdata is for a driver to store a pointer to driver specific data
>> 2) If no driver is bound, there is no driver specific data associated with
>>     the device
>> 3) Thus logically drvdata should be NULL if no driver is bound.
>>
>> I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
>> on probe failure.
>
> Hans,
>
> I believe this is a good thing per se, but shouldn't this rather be done
> in driver core, to guarantee that this hold across all buses?

Alan Stern said exactly the same thing :)

And I've just finished a patch doing exactly that. I'll send it to the
list right after this email.

Regards,

Hans

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Greg KH]

On Wed, May 23, 2012 at 12:00:37AM +0200, Jiri Kosina wrote:
> On Tue, 22 May 2012, Hans de Goede wrote:
> 
> > Well then lets work towards making it guaranteed, since I still believe
> > the following holds true:
> > 1) drvdata is for a driver to store a pointer to driver specific data
> > 2) If no driver is bound, there is no driver specific data associated with
> >    the device
> > 3) Thus logically drvdata should be NULL if no driver is bound.
> > 
> > I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
> > on probe failure.
> 
> Hans,
> 
> I believe this is a good thing per se, but shouldn't this rather be done 
> in driver core, to guarantee that this hold across all buses?

Maybe, but that would require that the driver core set the field to NULL
after every probe call that fails?  Is that really necessary to fix up
bugs in different bus subsystems that forget to clean up properly?

I'm not saying it's a bad thing to change it there, it just seems a bit
heavy-handed for a very rare event.

thanks,

greg k-h

Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Jiri Kosina]

On Tue, 22 May 2012, Greg KH wrote:

> > > Well then lets work towards making it guaranteed, since I still believe
> > > the following holds true:
> > > 1) drvdata is for a driver to store a pointer to driver specific data
> > > 2) If no driver is bound, there is no driver specific data associated with
> > >    the device
> > > 3) Thus logically drvdata should be NULL if no driver is bound.
> > > 
> > > I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
> > > on probe failure.
> > 
> > Hans,
> > 
> > I believe this is a good thing per se, but shouldn't this rather be done 
> > in driver core, to guarantee that this hold across all buses?
> 
> Maybe, but that would require that the driver core set the field to NULL
> after every probe call that fails?  Is that really necessary to fix up
> bugs in different bus subsystems that forget to clean up properly?
> 
> I'm not saying it's a bad thing to change it there, it just seems a bit
> heavy-handed for a very rare event.

The question is where does the requirement get imposed on the individual 
buses to actually reset drvdata? I don't think this has been 
'standardized', so resetting it in a driver core seems natural.

All in all, it has to be done anyway, so it makes sense to me to be done 
in as common code place as possible.

Thanks,

-- 
Jiri Kosina
SUSE Labs