linux-kernel - Re: code style: Re: [PATCH v4] 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: <YCwAbGoVuZJspcx5@chrisdown.name>
Date:   Tue, 16 Feb 2021 17:27:08 +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: code style: Re: [PATCH v4] printk: Userspace format enumeration
 support

Petr Mladek writes:
>I wonder if we could find a better name for the configure switch.
>I have troubles to imagine what printk enumeration might mean.
>Well, it might be because I am not a native speaker.
>
>Anyway, the word "enumeration" is used only in the configure option.
>Everything else is "printk_fmt"
>
>What about DEBUG_PRINTK_FORMATS?

Hmm, I don't like DEBUG_PRINTK_FMTS because it's not about debugging, it's 
about enumeration, I guess :-)

The name should reflect that this catalogues the available printks in the 
kernel -- "debugging" seems to imply something different.

I'm ok with a different name like "printk catalogue" or something like that if 
you prefer. Personally I think "printk enumeration" is fairly clear -- it's 
about enumerating the available printks -- but anything that captures that 
spirit is fine.

>printk.c is already too big. Please, implement this feature in a
>separate source file, e.g. kernel/printk/debug_formats.c.

Sure, that's fine.

>> struct printk_fmt_sec {
>> +	struct hlist_node hnode;
>> +	struct module *module;
>
>Please, use "struct module *mod". It is a more common.
>
>> +	struct dentry *file;
>> +	const char **start;
>> +	const char **end;
>> +};
>
>Please, document the meaning of the fields, ideally using the doc
>style or how is the style called:
>
>/**
> * struct printk_fmt_sec -
> * @hnode:	node for the hash table
> * @new_func:	pointer to the patched function code

Roger to both. :-)

>> +
>> +/* The base dir for module formats, typically debugfs/printk/formats/ */
>> +struct dentry *dfs_formats;
>> +
>> +/*
>> + * Stores .printk_fmt section boundaries for vmlinux and all loaded modules.
>> + * Add entries with store_printk_fmt_sec, remove entries with
>> + * remove_printk_fmt_sec.
>> + */
>> +static DEFINE_HASHTABLE(printk_fmts_mod_sections, 8);

>The hash table looks like an overkill. This is slow path. There are
>typically only tens of loaded modules. Even the module loader
>uses plain list for iterating the list of modules.

I don't think it's overkill -- we have prod systems with hundreds. Hell, even 
my laptop has over 150 loaded. If someone needs to walk all of the files in 
debugfs, it seems unreasonable to do an O(n^2) walk when an O(n) one would 
suffice.

Unless you have a practical concern, I think this is a distinct case from the 
module loader with its own unique requirements, so I'd prefer to use the hash 
table.

>> +
>> +/* Protects printk_fmts_mod_sections */
>> +static DEFINE_MUTEX(printk_fmts_mutex);
>
>What is the rule for using "printk_fmts" and "printk_fmt", please?
>I can't find the system here ;-)
>
>Anyway, I would prefer to use "printk_fmt" everywhere.
>Or maybe even "pf_".

pf_ sounds fine. :-)

>> +
>> +static const char *ps_get_module_name(const struct printk_fmt_sec *ps);
>> +static int debugfs_pf_open(struct inode *inode, struct file *file);
>
>There are used many different:
>
>   + shortcuts: fmt, fmts, ps, fmt_sec, dfs
>
>   + styles/ordering: ps_get_module_name() vs.
>		      find_printk_fmt_sec() vs.
>		      printk_fmt_size() vs.
>		      debugfs_pf_open()
>
>It might be bearable because there is not much code. But it would
>still help a lot when we make it more consistent. Many subsystems
>prefer using a feature-specific prefix.
>
>It might be "printk_fmt_" or "pf_" [*] in this case. And we could use
>names like:
>
>	+ struct pf_object [**]
>	+ pf_get_object_name()
>	+ pf_find_object()
>	+ pf_fops,
>	+ pf_open()
>	+ pf_release()
>	+ pf_mutex,
>	+ pf_add_object()
>	+ pf_remove_object()
>	+ pf_module_notify

Oh, I meant to change the name for v4 but neglected to do so. Sounds good, will 
do.