linux-kernel - Re: [PATCH -next] mm/hotplug: silence a lockdep splat with printk()

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: <20200114210215.GQ19428@dhcp22.suse.cz>
Date:   Tue, 14 Jan 2020 22:02:15 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     Qian Cai <cai@....pw>, akpm@...ux-foundation.org,
        sergey.senozhatsky.work@...il.com, pmladek@...e.com,
        rostedt@...dmis.org, peterz@...radead.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] mm/hotplug: silence a lockdep splat with printk()

On Tue 14-01-20 21:20:08, David Hildenbrand wrote:
> On 14.01.20 21:11, Qian Cai wrote:
> > Similar to the recent commit [1] merged into the random and -next trees,
> > it is not a good idea to call printk() with zone->lock held. The
> > standard fix is to use printk_deferred() in those places, but memory
> > offline will call dump_page() which need to defer after the lock. While
> > at it, remove a similar but unnecessary debug printk() as well.
> > 
> > [1] https://lore.kernel.org/lkml/1573679785-21068-1-git-send-email-cai@lca.pw/
> > 
> > Signed-off-by: Qian Cai <cai@....pw>
> > ---
> >  include/linux/page-isolation.h |  2 +-
> >  mm/memory_hotplug.c            |  2 +-
> >  mm/page_alloc.c                | 12 +++++-------
> >  mm/page_isolation.c            | 10 +++++++++-
> >  4 files changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> > index 148e65a9c606..5d8ba078006f 100644
> > --- a/include/linux/page-isolation.h
> > +++ b/include/linux/page-isolation.h
> > @@ -34,7 +34,7 @@ static inline bool is_migrate_isolate(int migratetype)
> >  #define REPORT_FAILURE	0x2
> >  
> >  bool has_unmovable_pages(struct zone *zone, struct page *page, int migratetype,
> > -			 int flags);
> > +			 int flags, char *dump);
> >  void set_pageblock_migratetype(struct page *page, int migratetype);
> >  int move_freepages_block(struct zone *zone, struct page *page,
> >  				int migratetype, int *num_movable);
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 7a6de9b0dcab..f10928538fa3 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1149,7 +1149,7 @@ static bool is_pageblock_removable_nolock(unsigned long pfn)
> >  		return false;
> >  
> >  	return !has_unmovable_pages(zone, page, MIGRATE_MOVABLE,
> > -				    MEMORY_OFFLINE);
> > +				    MEMORY_OFFLINE, NULL);
> >  }
> >  
> >  /* Checks if this range of memory is likely to be hot-removable. */
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index e56cd1f33242..b6bec3925e80 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -8204,7 +8204,7 @@ void *__init alloc_large_system_hash(const char *tablename,
> >   * race condition. So you can't expect this function should be exact.
> >   */
> >  bool has_unmovable_pages(struct zone *zone, struct page *page, int migratetype,
> > -			 int flags)
> > +			 int flags, char *dump)
> >  {
> >  	unsigned long iter = 0;
> >  	unsigned long pfn = page_to_pfn(page);
> > @@ -8305,8 +8305,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int migratetype,
> >  	return false;
> >  unmovable:
> >  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> > -	if (flags & REPORT_FAILURE)
> > -		dump_page(pfn_to_page(pfn + iter), reason);
> > +	if (flags & REPORT_FAILURE) {
> > +		page = pfn_to_page(pfn + iter);
> > +		strscpy(dump, reason, 64);
> > +	}
> >  	return true;
> >  }
> >  
> > @@ -8711,10 +8713,6 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> >  		BUG_ON(!PageBuddy(page));
> >  		order = page_order(page);
> >  		offlined_pages += 1 << order;
> > -#ifdef CONFIG_DEBUG_VM
> > -		pr_info("remove from free list %lx %d %lx\n",
> > -			pfn, 1 << order, end_pfn);
> > -#endif
> 
> ack to getting rid of this.

Yeah and that should go into it's own patch.

> Regarding the other stuff, I remember Michal had an opinion about the
> previous approach, so it's best if he comments.

Yeah, that was a long discussion with a lot of lockdep false positives.
I believe I have made it clear that the console code shouldn't depend on
memory allocation because that is just too fragile. If that is not
possible for some reason then it has to be mentioned in the changelog.
I really do not want us to add kludges to the MM code just because of
printk deficiencies unless that is absolutely inevitable.
-- 
Michal Hocko
SUSE Labs