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

[David Hildenbrand]

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