| 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
| ||
|
Message-ID: <0977bbdc-b862-acd6-3f55-bd04fd42215a@redhat.com>
Date: Wed, 16 Oct 2019 11:06:53 +0200
From: David Hildenbrand <david@...hat.com>
To: linux-kernel@...r.kernel.org
Cc: linux-mm@...ck.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Michal Hocko <mhocko@...nel.org>,
Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v1] drivers/base/memory.c: Drop the mem_sysfs_mutex
On 25.09.19 10:26, David Hildenbrand wrote:
> The mem_sysfs_mutex isn't really helpful. Also, it's not really clear what
> the mutex protects at all.
>
> The device lists of the memory subsystem are protected separately. We don't
> need that mutex when looking up. creating, or removing independent
> devices. find_memory_block_by_id() will perform locking on its own and
> grab a reference of the returned device.
>
> At the time memory_dev_init() is called, we cannot have concurrent
> hot(un)plug operations yet - we're still fairly early during boot. We
> don't need any locking.
>
> The creation/removal of memory block devices should be protected
> on a higher level - especially using the device hotplug lock to avoid
> documented issues (see Documentation/core-api/memory-hotplug.rst) - or
> if that is reworked, using similar locking.
>
> Protecting in the context of these functions only doesn't really make
> sense. Especially, if we would have a situation where the same memory
> blocks are created/deleted at the same time, there is something horribly
> going wrong (imagining adding/removing a DIMM at the same time from two
> call paths) - after the functions succeeded something else in the
> callers would blow up (e.g., create_memory_block_devices() succeeded but
> there are no memory block devices anymore).
>
> All relevant call paths (except when adding memory early during boot
> via ACPI, which is now documented) hold the device hotplug lock when
> adding memory, and when removing memory. Let's document that instead.
>
> Add a simple safety net to create_memory_block_devices() in case we
> would actually remove memory blocks while adding them, so we'll never
> dereference a NULL pointer. Simplify memory_dev_init() now that the
> lock is gone.
>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> Cc: Michal Hocko <mhocko@...nel.org>
> Cc: Oscar Salvador <osalvador@...e.de>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>
> Tested using my usual x86-64 DIMM based hot(un)plug setup.
>
> ---
> drivers/base/memory.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 6bea4f3f8040..634aab8e1e19 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -19,15 +19,12 @@
> #include <linux/memory.h>
> #include <linux/memory_hotplug.h>
> #include <linux/mm.h>
> -#include <linux/mutex.h>
> #include <linux/stat.h>
> #include <linux/slab.h>
>
> #include <linux/atomic.h>
> #include <linux/uaccess.h>
>
> -static DEFINE_MUTEX(mem_sysfs_mutex);
> -
> #define MEMORY_CLASS_NAME "memory"
>
> #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
> @@ -702,6 +699,8 @@ static void unregister_memory(struct memory_block *memory)
> * Create memory block devices for the given memory area. Start and size
> * have to be aligned to memory block granularity. Memory block devices
> * will be initialized as offline.
> + *
> + * Called under device_hotplug_lock.
> */
> int create_memory_block_devices(unsigned long start, unsigned long size)
> {
> @@ -715,7 +714,6 @@ int create_memory_block_devices(unsigned long start, unsigned long size)
> !IS_ALIGNED(size, memory_block_size_bytes())))
> return -EINVAL;
>
> - mutex_lock(&mem_sysfs_mutex);
> for (block_id = start_block_id; block_id != end_block_id; block_id++) {
> ret = init_memory_block(&mem, block_id, MEM_OFFLINE);
> if (ret)
> @@ -727,11 +725,12 @@ int create_memory_block_devices(unsigned long start, unsigned long size)
> for (block_id = start_block_id; block_id != end_block_id;
> block_id++) {
> mem = find_memory_block_by_id(block_id);
> + if (WARN_ON_ONCE(!mem))
> + continue;
> mem->section_count = 0;
> unregister_memory(mem);
> }
> }
> - mutex_unlock(&mem_sysfs_mutex);
> return ret;
> }
>
> @@ -739,6 +738,8 @@ int create_memory_block_devices(unsigned long start, unsigned long size)
> * Remove memory block devices for the given memory area. Start and size
> * have to be aligned to memory block granularity. Memory block devices
> * have to be offline.
> + *
> + * Called under device_hotplug_lock.
> */
> void remove_memory_block_devices(unsigned long start, unsigned long size)
> {
> @@ -751,7 +752,6 @@ void remove_memory_block_devices(unsigned long start, unsigned long size)
> !IS_ALIGNED(size, memory_block_size_bytes())))
> return;
>
> - mutex_lock(&mem_sysfs_mutex);
> for (block_id = start_block_id; block_id != end_block_id; block_id++) {
> mem = find_memory_block_by_id(block_id);
> if (WARN_ON_ONCE(!mem))
> @@ -760,7 +760,6 @@ void remove_memory_block_devices(unsigned long start, unsigned long size)
> unregister_memory_block_under_nodes(mem);
> unregister_memory(mem);
> }
> - mutex_unlock(&mem_sysfs_mutex);
> }
>
> /* return true if the memory block is offlined, otherwise, return false */
> @@ -794,12 +793,13 @@ static const struct attribute_group *memory_root_attr_groups[] = {
> };
>
> /*
> - * Initialize the sysfs support for memory devices...
> + * Initialize the sysfs support for memory devices. At the time this function
> + * is called, we cannot have concurrent creation/deletion of memory block
> + * devices, the device_hotplug_lock is not needed.
> */
> void __init memory_dev_init(void)
> {
> int ret;
> - int err;
> unsigned long block_sz, nr;
>
> /* Validate the configured memory block size */
> @@ -810,24 +810,19 @@ void __init memory_dev_init(void)
>
> ret = subsys_system_register(&memory_subsys, memory_root_attr_groups);
> if (ret)
> - goto out;
> + panic("%s() failed to register subsystem: %d\n", __func__, ret);
>
> /*
> * Create entries for memory sections that were found
> * during boot and have been initialized
> */
> - mutex_lock(&mem_sysfs_mutex);
> for (nr = 0; nr <= __highest_present_section_nr;
> nr += sections_per_block) {
> - err = add_memory_block(nr);
> - if (!ret)
> - ret = err;
> + ret = add_memory_block(nr);
> + if (ret)
> + panic("%s() failed to add memory block: %d\n", __func__,
> + ret);
> }
> - mutex_unlock(&mem_sysfs_mutex);
> -
> -out:
> - if (ret)
> - panic("%s() failed: %d\n", __func__, ret);
> }
>
> /**
>
Ping,
this lock does neither protect any data structure, nor is it helpful to
guarantee any ordering.
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists