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

[Rafael J. Wysocki]

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.