lkml.org 
[lkml]   [2012]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] driver core: fix shutdown races with probe/remove(v2)
    On Mon, Jun 18, 2012 at 09:52:57AM +0800, Ming Lei wrote:
    > On Sat, Jun 16, 2012 at 6:03 AM, Greg Kroah-Hartman
    > <gregkh@linuxfoundation.org> wrote:
    > > On Mon, Jun 11, 2012 at 01:13:20PM +0800, Ming Lei wrote:
    > >> Firstly, .shutdown callback may touch a uninitialized hardware
    > >> if dev->driver is set and .probe is not completed.
    > >>
    > >> Secondly, device_shutdown() may dereference a null pointer to cause
    > >> oops when dev->driver is cleared after it is checked in
    > >> device_shutdown().
    > >>
    > >> So just try to hold device lock and its parent lock(if it has) to
    > >> fix the races.
    > >>
    > >> Cc: Alan Stern <stern@rowland.harvard.edu>
    > >> Cc: stable@vger.kernel.org
    > >
    > > Why stable?  Are there known systems that crash right now without this
    > > change?  I don't think we ever heard back from the original poster about
    > > this issue as to what exactly was going wrong.
    >
    > I marked the patch as stable because it is really a fix on race between
    > shutdown and probe/remove, and the race can really happen in practice
    > as discussed in the thread. Once it happened, it will cause a big problem
    > on production machines.

    Have you read Documentation/stable_kernel_patches.txt? Please do so and
    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.

    > >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
    > >> ---
    > >> 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.
    Why not just do a real lock and try for forever?

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

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

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

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

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

    greg k-h
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-06-19 01:01    [W:0.045 / U:0.112 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site