linux-kernel - Re: [PATCH v5] printk: Userspace format enumeration support

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: <YEtyUM07gsqR6ltG@chrisdown.name>
Date:   Fri, 12 Mar 2021 13:53:20 +0000
From:   Chris Down <chris@...isdown.name>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        John Ogness <john.ogness@...utronix.de>,
        Johannes Weiner <hannes@...xchg.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>, kernel-team@...com
Subject: Re: [PATCH v5] printk: Userspace format enumeration support

Ack to all unmentioned suggestions. :-)

Petr Mladek writes:
>> +	  changed or no longer present.
>> +
>> +	  There is no additional runtime cost to printk with this enabled.
>> +
>>  #
>>  # Architectures with an unreliable sched_clock() should select this:
>>  #
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 1e5aad812310..44df2913a046 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1064,6 +1064,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>>  	blocking_notifier_call_chain(&module_notify_list,
>>  				     MODULE_STATE_GOING, mod);
>>  	klp_module_going(mod);
>> +	pi_sec_remove(mod);
>
>Is there any particular reason why this is not done via the module
>notifier, please?
>
>Other subsystems hardcode their callbacks here only when they
>require some special ordering that could not be achieved by
>the notifiers.
>
>The hardcoded callbacks complicate the error paths in
>the module loader code.

Oh! That's exactly what I feel as well, but I mistakenly thought that's what 
you were asking for in the feedback for v4. Turns out I misread your statement 
about storing the pointer to `struct module` (hence my message last time 
querying whether it was sensible or not) as being about not using the module 
notifier. Mea culpa.

>> +static void *pi_next(struct seq_file *s, void *v, loff_t *pos)
>> +{
>> +	const struct pi_sec *ps = s->file->f_inode->i_private;
>> +	struct pi_object *pi = NULL;
>
>Please, call the variables by the content and not by prefix.
>A variable called "pi" might include anything used by "pi" API.
>
>[...]
>
>Please, try to put more effort into creating the function and
>variable names. I know that I am probably too picky about it.
>But you seem to be the other extreme.
>
>Inconsistent, ambiguous, or meaningless names might make even few
>lines of code hard to follow. It makes it write-only.
>It is hard to review and maintain.

Hmm, I'd even say that I agree with this statement, but as I understand it a 
`pi` variable always means pi_object, and `ps` always means pi_sec. I'm not 
immediately seeing it as meaningless or ambiguous (although maybe your concern 
was more abstractly aesthetic with overlapping the `pi_` prefix?).

The "content" here is pretty abstract, so I'm not quite sure what your 
suggestion for naming them based on content is. Maybe (assuming it doesn't just 
disappear, which it seems it will) a pi_sec named sec, and the pi_object named 
fmt_index? I don't feel strongly that this is more clear though, so maybe you 
mean something else?