linux-kernel - Re: [PATCH v10 13/14] module: Move kdb module related code out of main kdb code

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, 8 Mar 2022 10:51:35 +0000
From:   Aaron Tomlin <atomlin@...hat.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "cl@...ux.com" <cl@...ux.com>, "mbenes@...e.cz" <mbenes@...e.cz>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "jeyu@...nel.org" <jeyu@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "void@...ifault.com" <void@...ifault.com>,
        "atomlin@...mlin.com" <atomlin@...mlin.com>,
        "allen.lkml@...il.com" <allen.lkml@...il.com>,
        "joe@...ches.com" <joe@...ches.com>,
        "msuchanek@...e.de" <msuchanek@...e.de>,
        "oleksandr@...alenko.name" <oleksandr@...alenko.name>,
        "jason.wessel@...driver.com" <jason.wessel@...driver.com>,
        "daniel.thompson@...aro.org" <daniel.thompson@...aro.org>,
        "hch@...radead.org" <hch@...radead.org>,
        "pmladek@...e.com" <pmladek@...e.com>
Subject: Re: [PATCH v10 13/14] module: Move kdb module related code out of
 main kdb code

On Tue 2022-03-08 08:36 +0000, Christophe Leroy wrote:
> 
> 
> Le 07/03/2022 à 18:47, Aaron Tomlin a écrit :
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 0852a537dad4..f3a30cd5037f 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -26,7 +26,6 @@
> >   #include <linux/utsname.h>
> >   #include <linux/vmalloc.h>
> >   #include <linux/atomic.h>
> > -#include <linux/module.h>
> >   #include <linux/moduleparam.h>
> >   #include <linux/mm.h>
> >   #include <linux/init.h>
> No need of linux/module.h here anymore ?

Hi Christophe,

Correct.

> In that case, I see several other files in kernel/debug/kdb/ that 
> include linux/module.h
> 
> Should it be removed in those files as well ?

I did not review the other kernel/debug/kdb/.*c files.
Anyhow, yes it can be removed from each, since it is entirely redundant.

> > diff --git a/kernel/module/kdb.c b/kernel/module/kdb.c
> > new file mode 100644
> > index 000000000000..60baeebea3e0
> > --- /dev/null
> > +++ b/kernel/module/kdb.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Module kdb support
> > + *
> > + * Copyright (C) 2010 Jason Wessel
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kdb.h>
> > +#include "internal.h"
> > +
> > +/*
> > + * kdb_lsmod - This function implements the 'lsmod' command.  Lists
> > + *	currently loaded kernel modules.
> > + *	Mostly taken from userland lsmod.
> > + */
> > +int kdb_lsmod(int argc, const char **argv)
> > +{
> > +	struct module *mod;
> > +
> > +	if (argc != 0)
> > +		return KDB_ARGCOUNT;
> > +
> > +	kdb_printf("Module                  Size  modstruct     Used by\n");
> > +	list_for_each_entry(mod, &modules, list) {
> > +		if (mod->state == MODULE_STATE_UNFORMED)
> > +			continue;
> > +
> > +		kdb_printf("%-20s%8u  0x%px ", mod->name,
> > +			   mod->core_layout.size, (void *)mod);
> > +#ifdef CONFIG_MODULE_UNLOAD
> > +		kdb_printf("%4d ", module_refcount(mod));
> > +#endif
> > +		if (mod->state == MODULE_STATE_GOING)
> > +			kdb_printf(" (Unloading)");
> > +		else if (mod->state == MODULE_STATE_COMING)
> > +			kdb_printf(" (Loading)");
> > +		else
> > +			kdb_printf(" (Live)");
> > +		kdb_printf(" 0x%px", mod->core_layout.base);
> > +
> > +#ifdef CONFIG_MODULE_UNLOAD
> > +		{
> > +			struct module_use *use;
> > +			kdb_printf(" [ ");
> > +			list_for_each_entry(use, &mod->source_list,
> > +					    source_list)
> > +				kdb_printf("%s ", use->target->name);
> > +			kdb_printf("]\n");
> > +		}
> > +#endif
> 
> That's a ugly construct. Could it be a function instead that you call 
> from this loop,

Fair enough and I agree; albeit, as you know, this was simply a migration
to kernel/module/kdb.c. We could indeed address this format/or style
concern later.


> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index b8a59b5c3e3a..bcc4f7a82649 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -108,10 +108,6 @@ static void mod_update_bounds(struct module *mod)
> >   		__mod_update_bounds(mod->init_layout.base, mod->init_layout.size);
> >   }
> >   
> > -#ifdef CONFIG_KGDB_KDB
> > -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
> 
> It should be removed from kernel/debug/kdb/kdb_private.h as well.

Agreed - this was missed.


Thanks,

-- 
Aaron Tomlin