linux-kernel - Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma

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: <2364837d5dcf80f3bdece75d8bfc2f9747467571.camel@perches.com>
Date:   Fri, 14 Aug 2020 21:18:35 -0700
From:   Joe Perches <joe@...ches.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        John Ogness <john.ogness@...utronix.de>
Subject: Re: [RFC PATCH] vsprintf: Add %pv extension replacement for
 print_vma_addr

On Sat, 2020-08-15 at 12:52 +0900, Sergey Senozhatsky wrote:
> Cc-ing John
> 
> On (20/08/14 10:53), Joe Perches wrote:
> [..]
> 
> In general, the idea looks nice.
> 
> > +static noinline_for_stack
> > +char *vma_addr(char *buf, char *end, void *ip,
> > +	       struct printf_spec spec, const char *fmt)
> > +{
> > +#ifdef CONFIG_MMU
> > +	struct mm_struct *mm = current->mm;
> > +	struct vm_area_struct *vma;
> > +
> > +	/*
> > +	 * we might be running from an atomic context so we cannot sleep
> > +	 */
> > +	if (!mmap_read_trylock(mm))
> > +		return buf;
> > +
> > +	vma = find_vma(mm, (unsigned long)ip);
> > +	if (vma && vma->vm_file) {
> > +		char *p;
> > +		struct file *f = vma->vm_file;
> > +		char *page = (char *)__get_free_page(GFP_NOWAIT);
> 
> Hmm, this is huge. For the time being this is going to introduce lock
> inversion chains:
> 
> 	PRINTK -> printk_locks -> MM -> mm_locks
> 
> vs
> 	MM -> mm_locks -> PRINTK -> printk_locks
> 
> Another thing to mention is
> 
> 	PRINTK -> printk_locks -> MM (WANR_ON/etc) -> PRINTK
> 
> we are in printk_safe, currently, but that's going to change.
> 
> We might not be ready to take this as of now, but things can change
> once we drop printk_locks.
> 
> > +		if (page) {
> > +			p = file_path(f, page, PAGE_SIZE);
> > +			if (IS_ERR(p))
> > +				p = "?";
> > +			buf = string(buf, end, kbasename(p), default_str_spec);
> > +			buf = string_nocheck(buf, end, "[", default_str_spec);
> > +			buf = pointer_string(buf, end,
> > +					     (void *)vma->vm_start,
> > +					     default_hex_spec);
> > +			buf = string_nocheck(buf, end, "+", default_str_spec);
> > +			buf = pointer_string(buf, end,
> > +					     (void *)(vma->vm_end - vma->vm_start),
> > +					     default_hex_spec);
> > +			buf = string_nocheck(buf, end, "]", default_str_spec);
> > +			free_page((unsigned long)page);
> > +		}
> > +	}
> > +	mmap_read_unlock(mm);
> > +#else
> > +	buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec);
> 
> Hmm, we don't usually do that CONFIG_FOO=n thing, I think we just need
> to skip %pv and print nothing.

I don't.

Right now, include/linux/mm.h has

#ifdef CONFIG_MMU
void print_vma_addr(char *prefix, unsigned long rip);
#else
static inline void print_vma_addr(char *prefix, unsigned long rip)
{
}
#endif

and the 'v' can't be #ifdef'd in vsprintf/pointer()
otherwise %pv would fallthrough to

	return ptr_to_id(buf, end, ptr, spec);