linux-kernel - Re: [PATCH v2 resend] mm/memory_hotplug: Make unpopulated zones PCP structures unreachable during hot remove

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b2696b77-9fe0-714f-6bdd-efc267974cfa@redhat.com>
Date:   Mon, 12 Apr 2021 14:12:00 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Mel Gorman <mgorman@...hsingularity.net>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Oscar Salvador <osalvador@...e.de>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Alexander Duyck <alexanderduyck@...com>,
        Minchan Kim <minchan@...nel.org>,
        Michal Hocko <mhocko@...e.com>, Linux-MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 resend] mm/memory_hotplug: Make unpopulated zones PCP
 structures unreachable during hot remove

On 12.04.21 14:08, Mel Gorman wrote:
> zone_pcp_reset allegedly protects against a race with drain_pages
> using local_irq_save but this is bogus. local_irq_save only operates
> on the local CPU. If memory hotplug is running on CPU A and drain_pages
> is running on CPU B, disabling IRQs on CPU A does not affect CPU B and
> offers no protection.
> 
> This patch deletes IRQ disable/enable on the grounds that IRQs protect
> nothing and assumes the existing hotplug paths guarantees the PCP cannot be
> used after zone_pcp_enable(). That should be the case already because all
> the pages have been freed and there is no page to put on the PCP lists.
> 
> Signed-off-by: Mel Gorman <mgorman@...hsingularity.net>
> ---
> Resending for email address correction and adding lists
> 
> Changelog since v1
> o Minimal fix
> 
>   mm/page_alloc.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5e8aedb64b57..9bf0db982f14 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8952,12 +8952,9 @@ void zone_pcp_enable(struct zone *zone)
>   
>   void zone_pcp_reset(struct zone *zone)
>   {
> -	unsigned long flags;
>   	int cpu;
>   	struct per_cpu_pageset *pset;
>   
> -	/* avoid races with drain_pages()  */
> -	local_irq_save(flags);
>   	if (zone->pageset != &boot_pageset) {
>   		for_each_online_cpu(cpu) {
>   			pset = per_cpu_ptr(zone->pageset, cpu);
> @@ -8966,7 +8963,6 @@ void zone_pcp_reset(struct zone *zone)
>   		free_percpu(zone->pageset);
>   		zone->pageset = &boot_pageset;
>   	}
> -	local_irq_restore(flags);
>   }
>   
>   #ifdef CONFIG_MEMORY_HOTREMOVE
> 

This was originally introduced by 340175b7d14d ("mm/hotplug: free 
zone->pageset when a zone becomes empty").

I wonder why it was ever added. Could it be that drain_pages() could 
have been called from an interrupt handler (e.g., on concurrent CPU 
hotunplug of the current CPU?) back then while we are just about to 
remove it ourselves?

Anyhow, if we need some protection after setting the zone unpopulated 
(setting present_pages = 0), it doesn't seem like disabling local IRQs 
is the right thing to do.

Reviewed-by: David Hildenbrand <david@...hat.com>

-- 
Thanks,

David / dhildenb