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

[David Hildenbrand <david@redhat.com>]

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. Therefore, the device_hotplug_lock will
remain held by the removing thread. The thread stuck in the sysfs
attribute will fail to trylock the device_hotplug_lock and return.

Other sysfs attributes that don't do a lock_device_hotplug_sysfs() might
have to do a trylock/lock on the device_lock to synchronize properly.

Summary: This patch is not necessary.

-- 
Thanks,

David / dhildenb