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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0iJJfTjZ6mZRDJSkZp+Cn1+VjFz=D8nLM15r6WtTRa88w@mail.gmail.com>
Date:   Fri, 24 Jan 2020 10:38:53 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Saravana Kannan <saravanak@...gle.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH v1] driver core: check for dead devices before onlining/offlining

On Fri, Jan 24, 2020 at 10:09 AM David Hildenbrand <david@...hat.com> 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@...uxfoundation.org>
> >> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> >> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> >> Cc: Saravana Kannan <saravanak@...gle.com>
> >> Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> >> Cc: Dan Williams <dan.j.williams@...el.com>
> >> Cc: Michal Hocko <mhocko@...nel.org>
> >> Signed-off-by: David Hildenbrand <david@...hat.com>
> >> ---
> >>
> >> 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.