[PATCH] backports: backport drvdata = NULL core driver fixes

[PATCH] backports: backport drvdata = NULL core driver fixes

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>

The Linux kernel had tons of code which at times cleared the
drvdata upon probe failure or release. There are however a bunch
of drivers that didn't clear this.

Commit 0998d063 implmented clearing this upon device_release_driver()
and dealt with probe failure on driver_probe_device(). After this the
kernel was cleaned up separately with *tons* of patches to remove all
these driver specific settings given that the clearing is now done
internally by the device core.

Instead of ifdef'ing code back in for older code where it was properly
in place backport this by piggy backing the new required code upon the
calls used in place. There is a small race here upon device_release_driver()
but we can live with that theoretical race.

Due to the way we hack this backport we can't use a separate namespace
as we have with other symbols.

mcgrof@frijol ~/linux-stable (git::master)$ git describe --contains \
	0998d0631001288a5974afc0b2a5f568bcdecb4d
v3.6-rc1~99^2~14^2~17

commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
Author: Hans de Goede <hdegoede@redhat.com>
Date:   Wed May 23 00:09:34 2012 +0200

    device-core: Ensure drvdata = NULL when no driver is bound

    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.

    But many drivers don't clear drvdata on device_release, or set drvdata
    early on in probe and leave it set on probe error. Both of which results
    in a dangling pointer in drvdata.

    This patch enforce for drvdata to be NULL after device_release or on probe
    failure.

    Signed-off-by: Hans de Goede <hdegoede@redhat.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Tested with ckmake against next-20130618:

1   2.6.24              [  OK  ]
2   2.6.25              [  OK  ]
3   2.6.26              [  OK  ]
4   2.6.27              [  OK  ]
5   2.6.28              [  OK  ]
6   2.6.29              [  OK  ]
7   2.6.30              [  OK  ]
8   2.6.31              [  OK  ]
9   2.6.32              [  OK  ]
10  2.6.33              [  OK  ]
11  2.6.34              [  OK  ]
12  2.6.35              [  OK  ]
13  2.6.36              [  OK  ]
14  2.6.37              [  OK  ]
15  2.6.38              [  OK  ]
16  2.6.39              [  OK  ]
17  3.0.79              [  OK  ]
18  3.1.10              [  OK  ]
19  3.10-rc1            [  OK  ]
20  3.2.45              [  OK  ]
21  3.3.8               [  OK  ]
22  3.4.46              [  OK  ]
23  3.5.7               [  OK  ]
24  3.6.11              [  OK  ]
25  3.7.10              [  OK  ]
26  3.8.13              [  OK  ]
27  3.9.3               [  OK  ]

real    32m2.332s
user    860m23.688s
sys     121m20.840s

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Felix Fietkau <nbd@openwrt.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 backport/backport-include/linux/device.h |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/backport/backport-include/linux/device.h b/backport/backport-include/linux/device.h
index c2f80e2..ba55d0e 100644
--- a/backport/backport-include/linux/device.h
+++ b/backport/backport-include/linux/device.h
@@ -176,4 +176,23 @@ extern int dev_set_name(struct device *dev, const char *name, ...)
 			__attribute__((format(printf, 2, 3)));
 #endif
 
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(3,6,0)
+#define driver_probe_device(__drv, __dev)		\
+({							\
+ 	int ret;					\
+ 	ret = (driver_probe_device)(__drv, __dev);	\
+ 	if (ret)					\
+		dev_set_drvdata(__dev, NULL);		\
+ 	return ret;					\
+})
+
+#define device_release_driver(__dev)			\
+({							\
+ 	(device_release_driver)(__dev);			\
+ 	device_lock(__dev);				\
+ 	dev_set_drvdata(__dev, NULL);			\
+ 	device_unlock(__dev);				\
+})
+#endif /* LINUX_VERSION_CODE <= KERNEL_VERSION(3,6,0) */
+
 #endif /* __BACKPORT_DEVICE_H */
-- 
1.7.10.4

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Alan Stern]

On Thu, 18 Jul 2013, Luis R. Rodriguez wrote:

> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> 
> The Linux kernel had tons of code which at times cleared the
> drvdata upon probe failure or release. There are however a bunch
> of drivers that didn't clear this.
> 
> Commit 0998d063 implmented clearing this upon device_release_driver()
> and dealt with probe failure on driver_probe_device(). After this the
> kernel was cleaned up separately with *tons* of patches to remove all
> these driver specific settings given that the clearing is now done
> internally by the device core.
> 
> Instead of ifdef'ing code back in for older code where it was properly
> in place backport this by piggy backing the new required code upon the
> calls used in place. There is a small race here upon device_release_driver()
> but we can live with that theoretical race.
> 
> Due to the way we hack this backport we can't use a separate namespace
> as we have with other symbols.
> 
> mcgrof@frijol ~/linux-stable (git::master)$ git describe --contains \
> 	0998d0631001288a5974afc0b2a5f568bcdecb4d
> v3.6-rc1~99^2~14^2~17
> 
> commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
> Author: Hans de Goede <hdegoede@redhat.com>
> Date:   Wed May 23 00:09:34 2012 +0200
> 
>     device-core: Ensure drvdata = NULL when no driver is bound
> 
>     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.
> 
>     But many drivers don't clear drvdata on device_release, or set drvdata
>     early on in probe and leave it set on probe error. Both of which results
>     in a dangling pointer in drvdata.
> 
>     This patch enforce for drvdata to be NULL after device_release or on probe
>     failure.
> 
>     Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

This is not a very good idea.  Although setting drvdata to NULL allowed
a lot of code to be removed, it also exposed a bunch of hidden bugs --
drivers were using the drvdata value even after their remove function
returned.

Several commits have been applied to fix those bugs.  Finding and
backporting them and their dependencies will not be easy.

I suggest this patch not be applied.

Alan Stern

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Luis R. Rodriguez]

On Fri, Jul 19, 2013 at 7:17 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 18 Jul 2013, Luis R. Rodriguez wrote:
>
>> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>>
>> The Linux kernel had tons of code which at times cleared the
>> drvdata upon probe failure or release. There are however a bunch
>> of drivers that didn't clear this.
>>
>> Commit 0998d063 implmented clearing this upon device_release_driver()
>> and dealt with probe failure on driver_probe_device(). After this the
>> kernel was cleaned up separately with *tons* of patches to remove all
>> these driver specific settings given that the clearing is now done
>> internally by the device core.
>>
>> Instead of ifdef'ing code back in for older code where it was properly
>> in place backport this by piggy backing the new required code upon the
>> calls used in place. There is a small race here upon device_release_driver()
>> but we can live with that theoretical race.
>>
>> Due to the way we hack this backport we can't use a separate namespace
>> as we have with other symbols.
>>
>> mcgrof@frijol ~/linux-stable (git::master)$ git describe --contains \
>>       0998d0631001288a5974afc0b2a5f568bcdecb4d
>> v3.6-rc1~99^2~14^2~17
>>
>> commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
>> Author: Hans de Goede <hdegoede@redhat.com>
>> Date:   Wed May 23 00:09:34 2012 +0200
>>
>>     device-core: Ensure drvdata = NULL when no driver is bound
>>
>>     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.
>>
>>     But many drivers don't clear drvdata on device_release, or set drvdata
>>     early on in probe and leave it set on probe error. Both of which results
>>     in a dangling pointer in drvdata.
>>
>>     This patch enforce for drvdata to be NULL after device_release or on probe
>>     failure.
>>
>>     Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> This is not a very good idea.  Although setting drvdata to NULL allowed
> a lot of code to be removed, it also exposed a bunch of hidden bugs --
> drivers were using the drvdata value even after their remove function
> returned.

Eek, have we not SmPL'ify'd a proof yet to ensure code like this no
longer exists? Julia? :)

> Several commits have been applied to fix those bugs.

I actualy found *tons* !

> Finding and backporting them and their dependencies will not be easy.

May the SmPL proof be with us.

> I suggest this patch not be applied.

Thanks for the review Alan!

  Luis

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Luis R. Rodriguez]

On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
>> This is not a very good idea.  Although setting drvdata to NULL allowed
>> a lot of code to be removed, it also exposed a bunch of hidden bugs --
>> drivers were using the drvdata value even after their remove function
>> returned.
>
> Eek, have we not SmPL'ify'd a proof yet to ensure code like this no
> longer exists? Julia? :)

Come to think of it, perhaps we should require *proof* with SmPL like
this in future to avoid regressions ?

  Luis

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Julia Lawall]

On Fri, 19 Jul 2013, Luis R. Rodriguez wrote:

> On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> >> This is not a very good idea.  Although setting drvdata to NULL allowed
> >> a lot of code to be removed, it also exposed a bunch of hidden bugs --
> >> drivers were using the drvdata value even after their remove function
> >> returned.
> >
> > Eek, have we not SmPL'ify'd a proof yet to ensure code like this no
> > longer exists? Julia? :)
> 
> Come to think of it, perhaps we should require *proof* with SmPL like
> this in future to avoid regressions ?

Is it a concurrency problem?  SmPL is not so good at that in the general 
case.  One would have to know a specific case where other functions of the 
driver can be invoked after remove.

julia

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Luis R. Rodriguez]

On Fri, Jul 19, 2013 at 2:27 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 19 Jul 2013, Luis R. Rodriguez wrote:
>
>> On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> >> This is not a very good idea.  Although setting drvdata to NULL allowed
>> >> a lot of code to be removed, it also exposed a bunch of hidden bugs --
>> >> drivers were using the drvdata value even after their remove function
>> >> returned.
>> >
>> > Eek, have we not SmPL'ify'd a proof yet to ensure code like this no
>> > longer exists? Julia? :)
>>
>> Come to think of it, perhaps we should require *proof* with SmPL like
>> this in future to avoid regressions ?
>
> Is it a concurrency problem?  SmPL is not so good at that in the general
> case.  One would have to know a specific case where other functions of the
> driver can be invoked after remove.

Thanks Julia. In that case I'm going to just leave this in place given
that if there's a bug upstream we'll get it fixed as soon as a
respective patch gets upstream as well. That is, we are not using old
drivers, we use the same upstream drivers so if a regression was found
in backports the fix must go upstream s well. This is one of the
benefits of backporting -- the range of users and testers increases
and we still benefit from the upstream bandwagon.

  Luis

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Alan Stern]

On Fri, 19 Jul 2013, Luis R. Rodriguez wrote:

> Thanks Julia. In that case I'm going to just leave this in place given
> that if there's a bug upstream we'll get it fixed as soon as a
> respective patch gets upstream as well. That is, we are not using old
> drivers, we use the same upstream drivers so if a regression was found
> in backports the fix must go upstream s well. This is one of the
> benefits of backporting -- the range of users and testers increases
> and we still benefit from the upstream bandwagon.

I don't understand.  If you're not using old drivers, and you
incorporate all the upstream patches, then what's the difference
between a backport and the current kernel?  In fact, if you're
incorporating all the upstream driver patches, then why haven't you
already got the drvdata change?

As one example of the sort of subtle problem exposed by the drvdata
change, take a look at commit b2ca69907657.

For more examples, see commits bf90ff5f3b8f, 638b9e15233c,
51ef847df746, 289b076f89c2, 53636555b919, 99a6f73c495c, 003615302a16,
94ab71ce2889, 3124d1d71d3d, c27f3efc5608, 95940a04bfe8, 5c1a0f418d8d,
db5c8b524444, 8bf769eb5f6e, 4295fe7791a1, fa919751a2d2, a9556040119a,
7bdce71822f4, and a1028f0abfb3.  Admittedly, these are all related
problems in a single subsystem, but it gives you a little idea of how 
far this goes.

Alan Stern

Re: [PATCH] backports: backport drvdata = NULL core driver fixes

[Luis R. Rodriguez]

On Fri, Jul 19, 2013 at 8:36 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 19 Jul 2013, Luis R. Rodriguez wrote:
>
>> Thanks Julia. In that case I'm going to just leave this in place given
>> that if there's a bug upstream we'll get it fixed as soon as a
>> respective patch gets upstream as well. That is, we are not using old
>> drivers, we use the same upstream drivers so if a regression was found
>> in backports the fix must go upstream s well. This is one of the
>> benefits of backporting -- the range of users and testers increases
>> and we still benefit from the upstream bandwagon.
>
> I don't understand.  If you're not using old drivers, and you
> incorporate all the upstream patches, then what's the difference
> between a backport and the current kernel?

The difference is that in older kernels driver_probe_device() and
device_release_driver() wouldn't do the setting to NULL in case of
probe fail or release, that's what the patch does. Given that the new
drivers have the superfluous setting removed we'd need a way to get
older kernels to clear it as well and there were two ways to do it --
revert the changes for all drivers this was cleared from or hack up
the same callback used driver_probe_device() and
device_release_driver() to do the appropriate clearing prior to
calling the old kernel's routines.

> In fact, if you're
> incorporating all the upstream driver patches, then why haven't you
> already got the drvdata change?

Because older kernel's driver_probe_device() and
device_release_driver() would be used.

> As one example of the sort of subtle problem exposed by the drvdata
> change, take a look at commit b2ca69907657.

Understood.

> For more examples, see commits bf90ff5f3b8f, 638b9e15233c,
> 51ef847df746, 289b076f89c2, 53636555b919, 99a6f73c495c, 003615302a16,
> 94ab71ce2889, 3124d1d71d3d, c27f3efc5608, 95940a04bfe8, 5c1a0f418d8d,
> db5c8b524444, 8bf769eb5f6e, 4295fe7791a1, fa919751a2d2, a9556040119a,
> 7bdce71822f4, and a1028f0abfb3.  Admittedly, these are all related
> problems in a single subsystem, but it gives you a little idea of how
> far this goes.

Sure, I understand. By pushing the newer drivers as-is *and* by
providing the driver_probe_device() and device_release_driver()
respective changes for older kernels it should get us similar behavior
on older kernels. Mind you, there is a small race in the way I
implemented this on device_release_driver() but that seems like a
reasonable tradeoff against all other alternatives I could come up
with.

As it is now, unless my port is incorrect, the newer drivers used on
older systems should trigger a similar bug as if using the upstream
kernel with the same drivers.

  Luis