linux-kernel - Re: [PATCH -next v2] 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]

Date:   Wed, 15 Jan 2020 10:22:21 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Qian Cai <cai@....pw>
Cc:     akpm@...ux-foundation.org, sergey.senozhatsky.work@...il.com,
        pmladek@...e.com, rostedt@...dmis.org, peterz@...radead.org,
        david@...hat.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next v2] mm/hotplug: silence a lockdep splat with
 printk()

On Wed 15-01-20 00:31:30, Qian Cai wrote:
> It is guaranteed to trigger a lockdep splat if calling printk() with
> zone->lock held because there are many places (tty, console drivers,
> debugobjects etc) would allocate some memory with another lock
> held which is proved to be difficult to fix them all.

This really should mention that most of them are false positives due to
early code intialization which cannot really cause a real lockup. AFAIR
you have also found some that really do allocate (GFP_ATOMIC) from the
console callback and those should be really fixed IMHO.

> A common workaround until the onging effort to make all printk() as
> deferred happens is to use printk_deferred() in those places similar to
> the recent commit [1] merged into the random and -next trees, but memory
> offline will call dump_page() which needs to be deferred after the lock.
> 
> So change has_unmovable_pages() so that it no longer calls dump_page()
> itself - instead it returns a "struct page *" of the unmovable page back
> to the caller so that in the case of a has_unmovable_pages() failure,
> the caller can call dump_page() after releasing zone->lock. Also, make
> dump_page() is able to report a CMA page as well, so the reason string
> from has_unmovable_pages() can be removed.

OK, this is slightly better than your previous attempts. Returing the
page without holding a reference is a quite subtle though. It should be
safe here because the page cannot go away because it is unmovable but
please add a comment that explains that the page _must not_ be used for
anything else than dumping its state.

> While at it, remove a similar but unnecessary debug-only printk() as
> well.

Because it doesn't really provide any useful information from the
practice.
[...]

> @@ -74,9 +75,9 @@ void __dump_page(struct page *page, const char *reason)
>  			page->mapping, page_to_pgoff(page),
>  			compound_mapcount(page));
>  	else
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx cma:%d\n",
>  			page, page_ref_count(page), mapcount,
> -			page->mapping, page_to_pgoff(page));
> +			page->mapping, page_to_pgoff(page), page_cma);

Is this correct? CMA pages cannot be comound? Btw. I would simply do

		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx%s\n",
			...., page_cmap ? "CMA": "");
-- 
Michal Hocko
SUSE Labs