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

[Rasmus Villemoes]

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.

> 
>>>> +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.

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

Rasmus