linux-kernel - Re: [PATCH v1] drivers/acpi/scan.c: Fixup "acquire device_hotplug_lock in acpi_scan

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: <20190731131408.GP9330@dhcp22.suse.cz>
Date:   Wed, 31 Jul 2019 15:14:08 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-acpi@...r.kernel.org,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Oscar Salvador <osalvador@...e.de>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v1] drivers/acpi/scan.c: Fixup "acquire
 device_hotplug_lock in acpi_scan_init()"

On Wed 31-07-19 15:02:49, David Hildenbrand wrote:
> On 31.07.19 14:53, Michal Hocko wrote:
> > On Wed 31-07-19 14:32:01, David Hildenbrand wrote:
> >> Let's document why we take the lock here. If we're going to overhaul
> >> memory hotplug locking, we'll have to touch many places - this comment
> >> will help to clairfy why it was added here.
> > 
> > And how exactly is "lock for consistency" comment going to help the poor
> > soul touching that code? How do people know that it is safe to remove it?
> > I am not going to repeat my arguments how/why I hate "locking for
> > consistency" (or fun or whatever but a real synchronization reasons)
> > but if you want to help then just explicitly state what should done to
> > remove this lock.
> > 
> 
> I know that you have a different opinion here. To remove the lock,
> add_memory() locking has to be changed *completely* to the point where
> we can drop the lock from the documentation of the function (*whoever
> knows what we have to exactly change* - and I don't have time to do that
> *right now*).

Not really. To remove a lock in this particular path it would be
sufficient to add
	/*
	 * Although __add_memory used down the road is documented to
	 * require lock_device_hotplug, it is not necessary here because
	 * this is an early code when userspace or any other code path
	 * cannot trigger hotplug operations.
	 */

Now that is a useful comment because it documents an exception and gives
you reasoning. If the above statement ever turns out to be incorrect due
to later changes then you can replace it with the lock and the new
reasoning.

But "just for consistency argument" doesn't tell you much when
scratching your head in the future and trying to figure out whether that
consistency argument still applies or there are new reasons the lock is
still needed.
-- 
Michal Hocko
SUSE Labs