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

[David Hildenbrand]

On 24.01.20 10:38, Rafael J. Wysocki wrote:
> On Fri, Jan 24, 2020 at 10:09 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>>
>> On 24.01.20 10:00, Greg Kroah-Hartman wrote:
>>> On Mon, Jan 20, 2020 at 11:49:09AM +0100, 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.
>>>>
>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>>>> Cc: Saravana Kannan <saravanak@xxxxxxxxxx>
>>>> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>>>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>>>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>>>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>>>> ---
>>>>
>>>> Am I missing any obvious mechanism in the device core that handles
>>>> something like this already? (especially also for other sysfs attributes?)
>>>
>>> So is a sysfs attribute causing the device itself to go away?  We have
>>
>> nope, removal is triggered via the driver, not via a sysfs attribute.
>>
>> Regarding this patch: Is there anything prohibiting the possible
>> scenario I document above (IOW, is this patch applicable, or is there
>> another way to fence it properly (e.g., the "specific call" you mentioned))?
> 
> For the devices that support online/offline (like CPUs in memory), the
> idea is that calling device_del() on them while they are in use may
> cause problems like data loss to occur and note that device_del()
> itself cannot fail (because it needs to handle surprise removal too).
> However, offline can fail, so the rule of thumb is to do the offline
> (and handle the errors possibly returned by it) and only call
> device_del() if that is successful.
> 
> Of course, if surprise removal is possible, offline is kind of
> pointless, but if it is supported anyway it should return success when
> the device is physically not present already.

Just to clarify: This is not about removing devices that are still
online (surprise removal). This is about possible race with user space
*while* removing an offline device. I think Greg pointed me into the
right direction regarding avoiding that ...


-- 
Thanks,

David / dhildenb