Re: [PATCH v1] driver core: check for dead devices before onlining/offlining

[Dan Williams]

On Fri, Jan 24, 2020 at 9:14 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 20.01.20 11:49, David Hildenbrand wrote:
> > We can have rare cases where the removal of a device races with
> > somebody trying to online it (esp. via sysfs). We can simply check
> > if the device is already removed or getting removed under the dev->lock.
> >
> > E.g., right now, if memory block devices are removed (remove_memory()),
> > we do a:
> >
> > remove_memory() -> lock_device_hotplug() -> mem_hotplug_begin() ->
> > lock_device() -> dev->dead = true
> >
> > Somebody coming via sysfs (/sys/devices/system/memory/memoryX/online)
> > triggers a:
> >
> > lock_device_hotplug_sysfs() -> device_online() -> lock_device() ...
> >
> > So if we made it just before the lock_device_hotplug_sysfs() but get
> > delayed until remove_memory() released all locks, we will continue
> > taking locks and trying to online the device - which is then a zombie
> > device.
> >
> > Note that at least the memory onlining path seems to be protected by
> > checking if all memory sections are still present (something we can then
> > get rid of). We do have other sysfs attributes
> > (e.g., /sys/devices/system/memory/memoryX/valid_zones) that don't do any
> > such locking yet and might race with memory removal in a similar way. For
> > these users, we can then do a
> >
> > device_lock(dev);
> > if (!device_is_dead(dev)) {
> >       /* magic /*
> > }
> > device_unlock(dev);
> >
> > Introduce and use device_is_dead() right away.
> >
>
> So, I just added the following:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 01cd06eeb513..49c4d8671073 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1567,6 +1567,7 @@ static ssize_t online_store(struct device *dev,
> struct device_attribute *attr,
>         if (ret < 0)
>                 return ret;
>
> +       msleep(10000);
>         ret = lock_device_hotplug_sysfs();
>         if (ret)
>                 return ret;
>
> Then triggered
>         echo 1 > /sys/devices/system/memory/memory51/online
> And quickly afterwards unplugged the DIMM.
>
> Good news is that we get (after 10 seconds)
>         sh: echo: write error: No such device
>
> Reason is that unplug will not finish before all sysfs attributes have
> been exited by other threads.

The unplug thread gets blocked for 10 seconds waiting for this thread
in online_store() to exit?