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

[Dan Williams]

On Fri, Jan 24, 2020 at 10:58 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
>
>
> > Am 24.01.2020 um 19:41 schrieb Dan Williams <dan.j.williams@xxxxxxxxx>:
> >
> > ï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?
> >
>
> Yes, thatâs what I observed.

Nice. This obviously wasn't in my mental model of what sysfs/kernfs guaranteed.

/me digs a bit...

So it's kernfs_drain() that is saving us. That waits for all active
nodes to deactivate and that guarantees that removal can be used as a
barrier. It also indicates that the lockdep splat I saw can't trigger
a deadlock unless mem_hotplug_begin() is being acquired within a
memblock-sysfs ops handler that is trying to remove memory. In all
cases I can audit it's always a 3rd-party sysfs attribute triggering
that removal.

So, simply moving remove_memory_block_devices() outside of
mem_hotplug_begin() should be a sufficient fix, no other invalidation
data needed. I'll respin that fix.