Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 15 Aug 2020 12:52:28 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
To:     Joe Perches <joe@perches.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Petr Mladek <pmladek@suse.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        John Ogness <john.ogness@linutronix.de>
Subject: Re: [RFC PATCH] vsprintf: Add %pv extension replacement for
 print_vma_addr
Message-ID: <20200815035228.GC582@jagdpanzerIV.localdomain>
References: <09f11651f0e913e159b955ac447cd8cadf36cb0d.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <09f11651f0e913e159b955ac447cd8cadf36cb0d.camel@perches.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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. Otherwise on !CONFIG_MMU systems the logbuf
may contain a number of CONFIG_MMU=n messages, which are hardly useful.

> +#endif
> +	return buf;
> +}
> +
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -2254,6 +2304,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		return uuid_string(buf, end, ptr, spec, fmt);
>  	case 'V':
>  		return va_format(buf, end, ptr, spec, fmt);

+ #ifdef CONFIG_MMU
> +	case 'v':
> +		return vma_addr(buf, end, ptr, spec, fmt);
+ #endif

>  	case 'K':
>  		return restricted_pointer(buf, end, ptr, spec);
>  	case 'N':

	-ss