linux-kernel - Re: [PATCH v3 3/4] module: add debug stats to help identify memory pressure

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]

Date:   Tue, 18 Apr 2023 11:30:36 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Petr Pavlu <petr.pavlu@...e.com>
Cc:     christophe.leroy@...roup.eu, tglx@...utronix.de,
        peterz@...radead.org, song@...nel.org, rppt@...nel.org,
        dave@...olabs.net, willy@...radead.org, vbabka@...e.cz,
        mhocko@...e.com, dave.hansen@...ux.intel.com,
        colin.i.king@...il.com, jim.cromie@...il.com,
        catalin.marinas@....com, jbaron@...mai.com,
        rick.p.edgecombe@...el.com, david@...hat.com,
        patches@...ts.linux.dev, linux-modules@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org, pmladek@...e.com,
        prarit@...hat.com, torvalds@...ux-foundation.org,
        gregkh@...uxfoundation.org, rafael@...nel.org
Subject: Re: [PATCH v3 3/4] module: add debug stats to help identify memory
 pressure

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