linux-kernel - Re: [PATCH 05/11] mm: debug: dump page into a string rather than directly on screen

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: <alpine.DEB.2.10.1506301627030.5359@chino.kir.corp.google.com>
Date:	Tue, 30 Jun 2015 16:35:45 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Sasha Levin <sasha.levin@...cle.com>
cc:	linux-mm@...ck.org, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, kirill@...temov.name
Subject: Re: [PATCH 05/11] mm: debug: dump page into a string rather than
 directly on screen

On Thu, 14 May 2015, Sasha Levin wrote:

> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 202ebdf..8b3f5a0 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -7,9 +7,7 @@ struct page;
>  struct vm_area_struct;
>  struct mm_struct;
>  
> -extern void dump_page(struct page *page, const char *reason);
> -extern void dump_page_badflags(struct page *page, const char *reason,
> -			       unsigned long badflags);
> +char *format_page(struct page *page, char *buf, char *end);
>  
>  #ifdef CONFIG_DEBUG_VM
>  char *format_vma(const struct vm_area_struct *vma, char *buf, char *end);
> @@ -18,7 +16,7 @@ char *format_mm(const struct mm_struct *mm, char *buf, char *end);
>  #define VM_BUG_ON_PAGE(cond, page)					\
>  	do {								\
>  		if (unlikely(cond)) {					\
> -			dump_page(page, "VM_BUG_ON_PAGE(" __stringify(cond)")");\
> +			pr_emerg("%pZp", page);				\
>  			BUG();						\
>  		}							\
>  	} while (0)
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 595bf50..1f045ae 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1382,6 +1382,8 @@ char *mm_pointer(char *buf, char *end, const void *ptr,
>  	switch (fmt[1]) {
>  	case 'm':
>  		return format_mm(ptr, buf, end);
> +	case 'p':
> +		return format_page(ptr, buf, end);
>  	case 'v':
>  		return format_vma(ptr, buf, end);
>  	default:
> @@ -1482,9 +1484,10 @@ int kptr_restrict __read_mostly;
>   *        (legacy clock framework) of the clock
>   * - 'Cr' For a clock, it prints the current rate of the clock
>   * - 'T' task_struct->comm
> - * - 'Z[mv]' Outputs a readable version of a type of memory management struct:
> + * - 'Z[mpv]' Outputs a readable version of a type of memory management struct:
>   *		v struct vm_area_struct
>   *		m struct mm_struct
> + *		p struct page
>   *
>   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>   * function pointers are really function descriptors, which contain a
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index fcad832..88b3cae 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -187,7 +187,7 @@ void balloon_page_putback(struct page *page)
>  		put_page(page);
>  	} else {
>  		WARN_ON(1);
> -		dump_page(page, "not movable balloon page");
> +		pr_alert("Not movable balloon page:\n%pZp", page);
>  	}
>  	unlock_page(page);
>  }

I don't know how others feel, but this looks strange to me and seems like 
it's only a result of how we must now dump page information 
(dump_page(page) is no longer available, we must do pr_alert("%pZp", 
page)).

Since we're relying on print formats, this would arguably be better as

	pr_alert("Not movable balloon page:\n");
	pr_alert("%pZp", page);

to avoid introducing newlines into potentially lengthy messages that need 
a specified loglevel like you've done above.

But that's not much different than the existing dump_page() 
implementation.

So for this to be worth it, it seems like we'd need a compelling usecase 
for something like pr_alert("%pZp %pZv", page, vma) and I'm not sure we're 
ever actually going to see that.  I would argue that

	dump_page(page);
	dump_vma(vma);

would be simpler in such circumstances.

I do understand the problem with the current VM_BUG_ON_PAGE() and 
VM_BUG_ON_VMA() stuff, and it compels me to ask about just going back to 
the normal

	VM_BUG_ON(cond);

coupled with dump_page(), dump_vma(), dump_whatever().  It all seems so 
much simpler to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/