Re: [PATCH v6 3/4] printk: Userspace format indexing support

[Andy Shevchenko]

On Wed, May 19, 2021 at 08:59:06AM +0200, Rasmus Villemoes wrote:
> On 18/05/2021 18.00, Andy Shevchenko wrote:
> > On Tue, May 18, 2021 at 03:07:44PM +0100, Chris Down wrote:
> >> Andy Shevchenko writes:

...

> >>>> +	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?
> > 
> > I have heard that modern GCC (at least) can utilize same constant literals in a
> > single compilation unit, so it won't be duplicated.
> 
> Yes, except it's not gcc but ld, string deduplication happens across
> compilation units, and "modern" isn't required, SHF_STRINGS and
> SHF_MERGE have been part of the ELF spec for decades, with support in
> binutils landing around 2001-04-13 AFAICT.
> 
> IOW, don't uglify the code by introducing macros or const char[]
> objects. Using string literals is just fine.

What I'm talking about is two things:
 - is it guaranteed that the name is always the same?
 - is it guaranteed that nobody will make a mistake in the name when typing it
   over and over?

  ...

> >>>> +static int __init pi_init(void)
> > 
> >>> 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?
> > 
> > While it's true, it may help in these cases:
> >  1) getting things done in a clean way
> 
> Huh?
> 
> >  2) finding bugs during boot cycle
> 
> What bugs would code that doesn't get executed find?
> 
> >  3) (possibly) making better debugging in virtual environments
> 
> How?
> 
> >  4) (also possibly) clean up something which shouldn't be seen by the next
> >     (unsecure) kernel, like kexec.
> 
> Tearing down a few debugfs files wouldn't touch a lot of memory, the
> printk format strings are very unlikely to be sensitive, and I highly
> doubt __exit code is kept around and run at kexec time anyway.

I admit that I'm on a learning curve in this area, and perhaps it was unclear
from the above that the list I gave is what I think may or might be relevant.

> IOW, please do not bloat the kernel image with __exit code in things
> which cannot be built modular.

Why we have exitcall in the code which can't be modular? Is somebody going to
clean that up? (Ex. `git grep -w __exitcall`)

-- 
With Best Regards,
Andy Shevchenko