linux-kernel - Re: [PATCH v1 02/11] mm/memory_hotplug: enforce section granularity when 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]

Date:   Wed, 19 Aug 2020 14:37:43 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Wei Yang <richard.weiyang@...ux.alibaba.com>,
        Baoquan He <bhe@...hat.com>,
        Pankaj Gupta <pankaj.gupta.linux@...il.com>,
        Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v1 02/11] mm/memory_hotplug: enforce section granularity
 when onlining/offlining

On Wed 19-08-20 12:11:48, David Hildenbrand wrote:
> Already two people (including me) tried to offline subsections, because
> the function looks like it can deal with it. But we really can only
> online/offline full sections (e.g., we can only mark full sections
> online/offline via SECTION_IS_ONLINE).
> 
> Add a simple safety net that to document the restriction now. Current users
> (core and powernv/memtrace) respect these restrictions.

I do agree with the warning because it clarifies our expectations
indeed. Se below for more questions.

> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Wei Yang <richard.weiyang@...ux.alibaba.com>
> Cc: Baoquan He <bhe@...hat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@...il.com>
> Cc: Oscar Salvador <osalvador@...e.de>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  mm/memory_hotplug.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c781d386d87f9..6856702af68d9 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -801,6 +801,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>  	int ret;
>  	struct memory_notify arg;
>  
> +	/* We can only online full sections (e.g., SECTION_IS_ONLINE) */
> +	if (WARN_ON_ONCE(!nr_pages ||
> +			 !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION)))
> +		return -EINVAL;

This looks looks unnecessarily cryptic to me. Do you want to catch full
section operation that doesn't start at the usual section boundary? If
yes the above doesn't work work unless I am missing something.

Why don't you simply WARN_ON_ONCE(nr_pages % PAGES_PER_SECTION).
!nr_pages doesn't sound like something interesting to care about or why
do we care?

-- 
Michal Hocko
SUSE Labs