On Tue, Jun 19, 2012 at 6:25 AM, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > Have you read Documentation/stable_kernel_patches.txt? Please do so and I haven't read Documentation/stable_kernel_patches.txt, but read Documentation/stable_kernel_rules.txt, :-) > see why I can't take this patch for a stable tree. Note that no one has > ever reported this as a bug before, and the original poster ran away > never to be heard from again, so I really don't think it was a real > problem that people ever saw. If Documentation/stable_kernel_rules.txt is the correct doc for stable rule, looks reporter requirement isn't listed in the file, but the below can be found: - No "theoretical race condition" issues, unless an explanation of how the race can be exploited is also provided. so I marked it as -stable because I have explained how the race can be exploited in reality. > >> >> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx> >> >> --- >> >> v2: >> >> - take Alan's suggestion to use device_trylock to avoid >> >> hanging during shutdown by buggy device or driver >> >> - hold parent reference counter >> >> >> >> drivers/base/core.c | 32 ++++++++++++++++++++++++++++++++ >> >> 1 file changed, 32 insertions(+) >> >> >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> >> index 346be8b..f2fc989 100644 >> >> --- a/drivers/base/core.c >> >> +++ b/drivers/base/core.c >> >> @@ -1796,6 +1796,16 @@ out: >> >> } >> >> EXPORT_SYMBOL_GPL(device_move); >> >> >> >> +static int __try_lock(struct device *dev) >> >> +{ >> >> + int i = 0; >> >> + >> >> + while (!device_trylock(dev) && i++ < 100) >> >> + msleep(10); >> >> + >> >> + return i < 100; >> >> +} >> > >> > That's a totally arbritary time, why does this work and other times do >> > not? And what is this returning, if the lock was grabbed successfully? >> >> It is a timeout time and is 1sec now. If the lock can't be held in 1sec, the >> function will return 0, otherwise it will return 1 and indicates that the lock >> has been held successfully. > > My point is why 1 second? That's completly arbitrary and means nothing. 1 second is a estimated value, just like many other timeout values in kernel. For example, the timeout passed to usb_control_msg() is mostly estimated first, then may be adjusted later from some new report. Another example is one recent xhci fix commit: 622eb783fe(xHCI: Increase the timeout for controller save/restore state operation) the timeout value is just increased arbitrarily to adapt new device. I still have more many examples in kernel about timeout value... > Why not just do a real lock and try for forever? IMO, there are two advantages not just doing a real lock for forever: - avoiding buggy device/driver to hang the system - with trylock, we can log the buggy device so that it is a bit easier to troubleshoot the buggy drivers, suppose the bug is only triggered 1 time in one year or more > >> Considered device lock is often held during probe and release in most >> of situations, 1sec should be a sane value because it may be abnormal >> if one driver's probe or release lasts for more than 1sec. > > How do you know how long a probe takes? I know of some that take far > longer than 1 second, so your patch just failed there :( Could you share the device or driver so that a better timeout value is set firstly? Also, the timeout value is just valid for hotplug device. > >> Also taking trylock is to prevent buggy drivers from hanging system during >> shutdown. If the timeout is too large, it may prolong shutdown time in >> the situation. > > If a buggy driver hangs, then we fix the buggy driver. We have the > source, we can do that. The problem may be triggered one time for one year or more, without the log provided by trylock, it is a bit difficult to fix it. > >> I will appreciate it very much if you can suggest a better timeout value. > > None, spin forever, take a lock for real. > >> > What's with the __ naming? >> >> No special meaning, if is not allowed, I can remove the '__'. > > Please do, it makes no sense. OK. > >> > I really don't like this at all. >> > >> > >> >> + >> >> /** >> >> * device_shutdown - call ->shutdown() on each device to shutdown. >> >> */ >> >> @@ -1810,8 +1820,11 @@ void device_shutdown(void) >> >> * devices offline, even as the system is shutting down. >> >> */ >> >> while (!list_empty(&devices_kset->list)) { >> >> + int nonlocked; >> >> + >> >> dev = list_entry(devices_kset->list.prev, struct device, >> >> kobj.entry); >> >> + get_device(dev->parent); >> > >> > Why grab the parent reference? >> >> If it is not grabbed, device_del may happen after the line below >> >> spin_unlock(&devices_kset->list_lock); >> >> so use-after-free may be triggered because the parent's lock >> is to be locked/unlocked in this patch. > > Then document that. OK. > >> >> get_device(dev); >> >> /* >> >> * Make sure the device is off the kset list, in the >> >> @@ -1820,6 +1833,18 @@ void device_shutdown(void) >> >> list_del_init(&dev->kobj.entry); >> >> spin_unlock(&devices_kset->list_lock); >> >> >> >> + /* hold lock to avoid race with .probe/.release */ >> >> + if (dev->parent && !__try_lock(dev->parent)) >> >> + nonlocked = 2; >> >> + else if (!__try_lock(dev)) >> >> + nonlocked = 1; >> >> + else >> >> + nonlocked = 0; >> > >> > Ick ick ick. Why can't we just grab the lock to try to only call these >> > callbacks one at a time? What is causing the big problem here that I am >> > missing? >> >> As discussed before in the thread, trylock is introduced to prevent buggy >> drivers from hanging system during shutdown. > > Fix buggy drivers, don't paper over them. As said above, the log may be very helpful for fixing the buggy drivers. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html