Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

From: Petr Mladek <pmladek@suse.com>
To: Timur Tabi <timur@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	Matthew Wilcox <willy@infradead.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, torvalds@linux-foundation.org,
	roman.fietze@magna.com, Steven Rostedt <rostedt@goodmis.org>,
	John Ogness <john.ogness@linutronix.de>,
	linux-mm@kvack.org, Akinobu Mita <akinobu.mita@gmail.com>
Subject: Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
Date: Wed, 20 Jan 2021 10:19:17 +0100	[thread overview]
Message-ID: <YAf1lS8zsHwhShCr@alley> (raw)
In-Reply-To: <29122c86-bfea-2f25-d111-00641cc660ba@kernel.org>

On Tue 2021-01-19 13:55:29, Timur Tabi wrote:
> On 1/19/21 1:45 PM, Kees Cook wrote:
> > How about this so the base address is hashed once, with the offset added
> > to it for each line instead of each line having a "new" hash that makes
> > no sense:
> > 
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 9301578f98e8..20264828752d 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >   		    const void *buf, size_t len, bool ascii)
> >   {
> >   	const u8 *ptr = buf;
> > +	const u8 *addr;
> >   	int i, linelen, remaining = len;
> >   	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> >   	if (rowsize != 16 && rowsize != 32)
> >   		rowsize = 16;
> > +	if (prefix_type == DUMP_PREFIX_ADDRESS &&
> > +	    ptr_to_hashval(ptr, &addr))
> > +		addr = 0;
> > +
> >   	for (i = 0; i < len; i += rowsize) {
> >   		linelen = min(remaining, rowsize);
> >   		remaining -= rowsize;
> > @@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >   		switch (prefix_type) {
> >   		case DUMP_PREFIX_ADDRESS:
> >   			printk("%s%s%p: %s\n",
> > -			       level, prefix_str, ptr + i, linebuf);
> > +			       level, prefix_str, addr + i, linebuf);
> 
> Well, this is better than nothing, but not by much.  Again, as long as %px
> exists for printk(), I just cannot understand any resistance to allowing it
> in print_hex_dump().
> 
> Frankly, I think this patch and my patch should both be added.  During
> debugging, it's very difficult if not impossible to work with hashed
> addresses.  I use print_hex_dump() with an unhashed address all the time,
> either by applying my patch to my own kernel or just replacing the %p with
> %px.

This was my view as well. People should not need to add features into
hexdump code when they want to use is for debugging. And the address
is pretty useful.

A solution might be to prevent using this in the mainline:

   + it might be reported by checkpatch.pl

   + it might print some bold warning on the first use, similar to
     trace_printk().

Or we need this in the mainline. Then the use of %pK looks
like the best compromise to me. We could also add some warning
as a comment and use some provocative name for the flag
as suggested by Matthew.

And we should definitely add Linus into CC when sending v2.
His expected opinion has been mentioned several times in this
thread. It would be better to avoid these speculations
and get his real opinion. IMHO, it is too late to add
him in this long thread.

Best Regards,
Petr