On Mon, Apr 17, 2023 at 01:18:14PM +0200, Petr Pavlu wrote:
> On 4/14/23 07:08, Luis Chamberlain wrote:
<-- Petr's spell checking -->
> Note that there are plenty of other typos in the added comments and
> documentation. Please review them with a spell checker.
Yes I am terrible at that, I've now integrated a spell checker into
my workflow. Fixed all these, thanks.
> > @@ -2500,6 +2503,18 @@ static noinline int do_init_module(struct module *mod)
> > {
> > int ret = 0;
> > struct mod_initfree *freeinit;
> > +#if defined(CONFIG_MODULE_STATS)
> > + unsigned int text_size = 0, total_size = 0;
> > +
> > + for_each_mod_mem_type(type) {
> > + const struct module_memory *mod_mem = &mod->mem[type];
> > + if (mod_mem->size) {
> > + total_size += mod_mem->size;
> > + if (type == MOD_TEXT || type == MOD_INIT_TEXT)
> > + text_size += mod->mem[type].size;
>
> 'text_size += mod_mem->size;' would be simpler.
Sure.
> > +extern struct dentry *mod_debugfs_root;
>
> Files kernel/module/stats.c and kernel/module/tracking.c both add this extern
> declaration. Can it be moved to kernel/module/internal.h?
Sure.
> > +#if defined(CONFIG_MODULE_DECOMPRESS)
> > + if (flags & MODULE_INIT_COMPRESSED_FILE)
> > + atomic_long_add(info->compressed_len, &invalid_mod_byte);
>
> Variable invalid_mod_byte is not declared, should be invalid_mod_bytes.
Arnd already sent a fix for that, thanks.
> > +int try_add_failed_module(const char *name, size_t len, enum fail_dup_mod_reason reason)
>
> Function try_add_failed_module() is only called from
> module_patient_check_exists() which always passes in a NUL-terminated string.
> The len parameter could be then dropped and the comparison in
> try_add_failed_module() could simply use strcmp().
Sure, did that.
> Indentation in try_add_failed_module() uses spaces instead of tabs in a few
> places.
Fixed.
> > + size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming) * MAX_BYTES_PER_MOD,
> > + (unsigned int) MAX_FAILED_MOD_PRINT * MAX_BYTES_PER_MOD);
>
> Using
> 'size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming), (unsigned int)MAX_FAILED_MOD_PRINT) * MAX_BYTES_PER_MOD;'
> is a bit simpler and avoids any theoretical overflow of
> '(floads + fbecoming) * MAX_BYTES_PER_MOD'.
Sure.
> > + len += scnprintf(buf + len, size - len, "%25s\t%15s\t%25s\n",
> > + "module-name", "How-many-times", "Reason");
>
> "module-name" -> "Module-name"
OK sure.
> Function module_stats_init() requires mod_debugfs_root being initialized which
> is done in module_debugfs_init(). Both functions are recorded to be called via
> module_init(). Just to make sure, is their ordering guaranteed in some way?
Link order takes care of that and main.o goes first.
> mod_debugfs_root is initialized in module_debugfs_init() only if
> CONFIG_MODULE_DEBUG is set. However, my reading is that feature
> CONFIG_MODULE_UNLOAD_TAINT_TRACKING is orthogonal to it and doesn't require
> CONFIG_MODULE_DEBUG, so it looks this change breaks this tracking?
Ah yes We need a bool CONFIG_MODULE_DEBUGFS which is selected by those
that need it. Added.
Luis
