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:   Tue, 18 May 2021 15:07:44 +0100
From:   Chris Down <chris@chrisdown.name>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, Petr Mladek <pmladek@suse.com>,
        Jessica Yu <jeyu@kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        John Ogness <john.ogness@linutronix.de>,
        Steven Rostedt <rostedt@goodmis.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Kees Cook <keescook@chromium.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>, kernel-team@fb.com
Subject: Re: [PATCH v6 3/4] printk: Userspace format indexing support
Message-ID: <YKPKMCNz2hccaXfR@chrisdown.name>
References: <cover.1621338324.git.chris@chrisdown.name>
 <05d25c65d3f5149c1e8537f74041a7a46bd489d6.1621338324.git.chris@chrisdown.name>
 <YKPBk+7lTzs8WFAk@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <YKPBk+7lTzs8WFAk@smile.fi.intel.com>
User-Agent: Mutt/2.0.7 (481f3800) (2021-05-04)
Precedence: bulk

Hey Andy,

Thanks for the detailed feedback! All points not explicitly replied to here are 
acked and will be improved. :-)

Andy Shevchenko writes:
>> +static const char *pi_get_module_name(struct module *mod)
>> +{
>> +	return mod ? mod->name : "vmlinux";
>
>First of all, you have several occurrences of the "vmlinux" literal.
>Second, can't you get it from somewhere else? Is it even guaranteed that the
>name is always the same?

Hmm, I don't know if it's guaranteed, but we already have similar logic in (as 
one example) livepatch, which seems to suggest it's not obviously wrong:

     % grep -R '"vmlinux"' kernel/livepatch/
     kernel/livepatch/core.c:                       sympos, name, objname ? objname : "vmlinux");
     kernel/livepatch/core.c:        bool sec_vmlinux = !strcmp(sec_objname, "vmlinux");
     kernel/livepatch/core.c:                sym_vmlinux = !strcmp(sym_objname, "vmlinux");
     kernel/livepatch/core.c:        if (strcmp(objname ? objname : "vmlinux", sec_objname))
     kernel/livepatch/core.c:        name = klp_is_module(obj) ? obj->name : "vmlinux";
     kernel/livepatch/core.c:                                klp_is_module(obj) ? obj->name : "vmlinux");
     kernel/livepatch/core.c:                                klp_is_module(obj) ? obj->name : "vmlinux");
     kernel/livepatch/core.c:        if (!strcmp(mod->name, "vmlinux")) {

Is there another name or method you'd prefer? :-)

As for the literals, are you saying that you prefer that it's symbolised as a 
macro or static char, or do you know of an API where this kind of name can be 
canonically accessed?

>> +#define seq_escape_printf_format(s, src) \
>> +	seq_escape_str(s, src, ESCAPE_ANY | ESCAPE_NAP | ESCAPE_APPEND, "\"\\")
>
>Hmm... But after your ESCAPE_SPECIAL update why " is in @only?
>Not sure about back slash either.

Good question! It's because ESCAPE_NAP (used to reduce scope of ESCAPE_OCTAL) 
will cause double quote and backslash to be ignored for quoting otherwise, even 
with ESCAPE_SPECIAL from ESCAPE_ANY.

I touched on this briefly in the changelog for the patch adding the quote to 
ESCAPE_SPECIAL:

 From "string_helpers: Escape double quotes in escape_special":
> One can of course, alternatively, use ESCAPE_APPEND with a quote in
> @only, but without this patch quotes are coerced into hex or octal which
> can hurt readability quite significantly.

Maybe you know of a more intuitive way to deal with this? :-)

>> +static int __init pi_init(void)
>> +{
>> +	struct dentry *dfs_root = debugfs_create_dir("printk", NULL);
>> +
>> +	dfs_index = debugfs_create_dir("index", dfs_root);
>> +	pi_setup_module_notifier();
>> +	pi_create_file(NULL);
>> +
>> +	return 0;
>> +}
>
>No __exit? (There is a corresponding call for exit)

Hmm, can't printk only be built in to the kernel, so it can't be unloaded? At 
least it looks that way from Kconfig. Maybe I'm missing something and there's 
some other way that might be invoked?

Thanks a lot for the feedback! :-)