| Message ID | 20211222132332.7817-1-vimal.agrawal@sophos.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kernel/module.c: heuristic enhancement when INSTALL_MOD_STRIP= "--strip-unneeded" is used | expand |
On Wed, Dec 22, 2021 at 06:53:32PM +0530, Vimal Agrawal wrote: > If kernel modules are stripped off symbols (e.g. built by using strip > --strip-unneeded option) then stack traces in dmesg do not show symbol We never build modules with that ёption, so this is completely pointless. NAK.
Hi Christoph, On Thu, Dec 23, 2021 at 4:06 PM Christoph Hellwig <hch@infradead.org> wrote: > > We never build modules with that ёption, so this is completely pointless. > we use openwrt for build and packaging and it has been using this option for long. kbuild documentation says the following for INSTALL_MOD_STRIP: If this variable is specified, it will cause modules to be stripped after they are installed. If INSTALL_MOD_STRIP is ‘1’, then the default option –strip-debug will be used. Otherwise, the INSTALL_MOD_STRIP value will be used as the option(s) to the strip command. So if kbuild does not support INSTALL_MOD_STRIP=--strip-unneeded option then we should call out what it supports and should not even allow what is not supported. We don't know what other options others may be using but if we allow it then we should support it and it should not behave erratic just because someone is using a non-recommended option. Vimal > NAK.
On Thu, Dec 23, 2021 at 04:39:15PM +0530, Vimal Agrawal wrote: > Hi Christoph, > > On Thu, Dec 23, 2021 at 4:06 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > We never build modules with that ёption, so this is completely pointless. > > > we use openwrt for build and packaging and it has been using this > option for long. > > kbuild documentation says the following for INSTALL_MOD_STRIP: > If this variable is specified, it will cause modules to be stripped > after they are installed. If INSTALL_MOD_STRIP is ‘1’, then the > default option –strip-debug will be used. Otherwise, the > INSTALL_MOD_STRIP value will be used as the option(s) to the strip > command. > > So if kbuild does not support INSTALL_MOD_STRIP=--strip-unneeded > option then we should call out what it supports and should not even > allow what is not supported. We don't know what other options others > may be using but if we allow it then we should support it and it > should not behave erratic just because someone is using a > non-recommended option. I don't think we can support passing arbitrary linker options and expects things to work. If we want to support --strip-unneeded it needs a good rationale and be added as a direct config option.
On Fri, Dec 24, 2021 at 12:17 PM Christoph Hellwig <hch@infradead.org> wrote: > > I don't think we can support passing arbitrary linker options and > expects things to work. If we want to support --strip-unneeded > it needs a good rationale and be added as a direct config option. INSTALL_MOD_STRIP was provided to give flexibility for providing strippping options. It will be a separate discussion if we want to continue allowing this flexibility or not but it is available now. I see that it works but just that it is behaving erratically during symbol decodes. I tried few other stripping options. --strip-all does not work and insmod fails as it needs some symbols which is fine as it is hinting the user there is a problem with stripping option. This is not the case with --strip-unneeded option as it builds and loads/works fine but fails to decode symbols properly sometimes. One of the rationale for --strip-unneeded option is that it significantly reduces the size of .ko without affecting its functionality ( though debuggability takes a hit). I guess many platforms/products that need limited memory footprint might be using this option. I am fine if we decide to say that --strip-unneeded option is not supported by kbuild/INSTALL_MOD_STRIP which is not the case now. In that case, I agree that this patch will not even be required. Vimal
On Sat, Dec 25, 2021 at 06:38:02AM +0530, Vimal Agrawal wrote: > On Fri, Dec 24, 2021 at 12:17 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > I don't think we can support passing arbitrary linker options and > > expects things to work. If we want to support --strip-unneeded > > it needs a good rationale and be added as a direct config option. > INSTALL_MOD_STRIP was provided to give flexibility for providing > strippping options. Yes but the point here is the heuristic you are adding for when "--strip-unneeded" is used is now *always* being used and we have no way of knowing this. So I'd agree with Christoph that if we want to support this it might make sense to make a kconfig option for enabling "--strip-unneeded" and then another for this heuristic. You may want to work on top of Aaron's patches as that would make adding new kconfig entries for modules much cleaner: https://lkml.kernel.org/r/20220106234319.2067842-1-atomlin@redhat.com I hope to review those this week, so I might have a tree you can work off on by the end of this or next week. Luis
On Tue, Jan 11, 2022 at 9:19 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > Yes but the point here is the heuristic you are adding for > when "--strip-unneeded" is used is now *always* being used and > we have no way of knowing this. So I'd agree with Christoph that > if we want to support this it might make sense to make a kconfig > option for enabling "--strip-unneeded" and then another for this > heuristic. This heuristic is applicable to any case when an address inside a module can not be decoded to any known symbol. e.g. anyone can still build with ----strip-all though module load fails with this option. but one can add or remove symbols manually or use objcopy or some other utility to play with symbols. It does not matter for functionality much if symbols are available or not and it is just that symbol decodes in traces are not providing help as it displays absolute address in such cases. There are several options in strip command and we can't have kconfig for each such option. All options are supported currently unless the module is so broken that it can even be loaded ( e.g. --strip-all option). Vimal
On Wed, Jan 12, 2022 at 02:06:48PM +0530, Vimal Agrawal wrote: > On Tue, Jan 11, 2022 at 9:19 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > Yes but the point here is the heuristic you are adding for > > when "--strip-unneeded" is used is now *always* being used and > > we have no way of knowing this. So I'd agree with Christoph that > > if we want to support this it might make sense to make a kconfig > > option for enabling "--strip-unneeded" and then another for this > > heuristic. > > This heuristic is applicable to any case when an address inside a > module can not be decoded to any known symbol. e.g. You mean it is safe for that case too? If so can you add this to the commit log as well? > anyone can still > build with ----strip-all though module load fails with this option. > but one can add or remove symbols manually or use objcopy or some > other utility to play with symbols. It does not matter for > functionality much if symbols are available or not and it is just that > symbol decodes in traces are not providing help as it displays > absolute address in such cases. > > There are several options in strip command and we can't have kconfig > for each such option. This is a good point. Pointing out it is safe regardless of the situation in the commit log I think does this heuristic more justice. Can you resend with that being clarified? I'll still have to test this. Can you also use the modules-next tree? I'll still have to test this. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/ branch modules-next Luis > All options are supported currently unless the > module is so broken that it can even be loaded ( e.g. --strip-all > option). > > Vimal
Hi Luis, On Thu, Jan 13, 2022 at 8:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > You mean it is safe for that case too? If so can you add this to the > commit log as well? > yes. updated in the commit log. > Can you also use the modules-next tree? I'll still have to test this. > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/ branch modules-next > I am not sure of modules-next but I created the patch (v3) on latest linux-next (next-20220116). Hope it suffices. Sent patch v3 already. Vimal
diff --git a/kernel/module.c b/kernel/module.c index 320ec908045f..6e1621303156 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -4256,14 +4256,21 @@ static const char *find_kallsyms_symbol(struct module *mod, unsigned long *offset) { unsigned int i, best = 0; - unsigned long nextval, bestval; + unsigned long baseval, nextval, bestval; struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); + char *module_base_name; /* At worse, next value is at end of module */ - if (within_module_init(addr, mod)) + if (within_module_init(addr, mod)) { + baseval = (unsigned long)mod->init_layout.base; nextval = (unsigned long)mod->init_layout.base+mod->init_layout.text_size; - else + module_base_name = "[module __init]"; + + } else { + baseval = (unsigned long)mod->core_layout.base; nextval = (unsigned long)mod->core_layout.base+mod->core_layout.text_size; + module_base_name = "[module]"; + } bestval = kallsyms_symbol_value(&kallsyms->symtab[best]); @@ -4294,6 +4301,19 @@ static const char *find_kallsyms_symbol(struct module *mod, nextval = thisval; } + if ((is_module_text_address(addr) && + (bestval < baseval || bestval > nextval))) { + /* + * return MODULE base and offset if we could not find + * any best match for text address + */ + if (size) + *size = nextval - baseval; + if (offset) + *offset = addr - baseval; + return module_base_name; + } + if (!best) return NULL;
If kernel modules are stripped off symbols (e.g. built by using strip --strip-unneeded option) then stack traces in dmesg do not show symbol name for address. It just prints absolute address sometimes (if there is no good match with any symbol) e.g. [245864.699580] do_nmi+0x12f/0x370 [245864.699583] end_repeat_nmi+0x16/0x50 [245864.699585] RIP: 0010:0xffffffffc06b67ec <<<<<<<< [245864.699585] RSP: 0000:ffffaaa540cffe48 EFLAGS: 00000097 [245864.699586] RAX: 0000000000000001 RBX: ffff93357a729000 RCX: 0000000000000001 [245864.699587] RDX: ffff93357a729050 RSI: 0000000000000000 RDI: ffff93357a729000 [245864.699588] RBP: ffff9335cf521300 R08: 0000000000000001 R09: 0000000000000004 [245864.699588] R10: ffffaaa545b23ed0 R11: 0000000000000001 R12: ffffffffc06b61a0 [245864.699589] R13: ffffaaa540cffe60 R14: ffff9335c77fa3c0 R15: ffff9335cf51d7c0 [245864.699590] ? 0xffffffffc06b61a0 [245864.699592] ? 0xffffffffc06b67ec <<<<<<<< [245864.699593] ? 0xffffffffc06b67ec [245864.699594] </NMI> Note RIP: 0010:0xffffffffc06b67ec and 0xffffffffc06b67ec printed in above stack trace as absolute address. There is no easy way in case box crashes as we loose information on load address of specific module. This changes the symbol decoding (in kernel/ module.c) such that it can print offset from start of section (.text or .init.text) in case there is no good match with any symbol. It will now decode address in such cases to [module]+ offset/size or [module __init]+offset/size depending on where the address lies (in core/.text or init/.init.text section of module). One can use objdump/readelf/nm to find symbols with offset from .init.text and .text sections. steps to reproduce the problem: ------------------------------- 1. Add WARN_ON_ONCE(1) in module e.g. test_module.c 2. Build and strip the module using --strip-unneeded option 3. Load the module and check RIP in dmesg tests done: ----------- 1. Added WARN_ON_ONE(1) in functions of a module for testing ------------------------------------------------------------- [ 407.934085] CPU: 0 PID: 2956 Comm: insmod Tainted: G W E 5.16.0-rc5-next-20211220+ #2 [ 407.934087] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 407.934089] returning module_base_name: [module __init] [ 407.934088] RIP: 0010:[module __init]+0x4/0x7 [test_module] [ 407.934092] returning module_base_name: [module __init] [ 407.934097] Code: Unable to access opcode bytes at RIP 0xffffffffc07edfda. [ 407.934098] RSP: 0018:ffffb21440487c20 EFLAGS: 00010202 [ 407.934100] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 407.934101] RDX: 0000000000000000 RSI: ffffffff9c38e5e1 RDI: 0000000000000001 [ 407.934102] RBP: ffffb21440487c28 R08: 0000000000000000 R09: ffffb21440487a20 [ 407.934103] R10: ffffb21440487a18 R11: ffffffff9c755248 R12: ffffffffc07ee007 [ 407.934104] R13: ffff92a0f1e260b0 R14: 0000000000000000 R15: 0000000000000000 [ 407.934105] FS: 00007f578ebc4400(0000) GS:ffff92a1c0e00000(0000) knlGS:0000000000000000 [ 407.934107] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 407.934108] CR2: ffffffffc07edfda CR3: 00000000063ea006 CR4: 00000000000706f0 [ 407.934113] Call Trace: [ 407.934114] <TASK> [ 407.934116] ? init_module+0x55/0xff9 [test_module] ... [ 407.934232] CPU: 0 PID: 2956 Comm: insmod Tainted: G W E 5.16.0-rc5-next-20211220+ #2 [ 407.934234] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 407.934242] returning module_base_name: [module] [ 407.934242] RIP: 0010:[module]+0x4/0x7 [test_module] [ 407.934245] returning module_base_name: [module] [ 407.934248] Code: Unable to access opcode bytes at RIP 0xffffffffc07e1fda. [ 407.934249] RSP: 0018:ffffb21440487c20 EFLAGS: 00010202 [ 407.934251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 407.934252] RDX: 0000000000000000 RSI: ffffffff9c38e5e1 RDI: 0000000000000001 [ 407.934253] RBP: ffffb21440487c28 R08: 0000000000000000 R09: ffffb21440487a20 [ 407.934254] R10: ffffb21440487a18 R11: ffffffff9c755248 R12: ffffffffc07ee007 [ 407.934255] R13: ffff92a0f1e260b0 R14: 0000000000000000 R15: 0000000000000000 [ 407.934256] FS: 00007f578ebc4400(0000) GS:ffff92a1c0e00000(0000) knlGS:0000000000000000 [ 407.934257] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 407.934258] CR2: ffffffffc07e1fda CR3: 00000000063ea006 CR4: 00000000000706f0 [ 407.934260] Call Trace: [ 407.934260] <TASK> [ 407.934261] ? init_module+0x5a/0xff9 [test_module] note that it is able to decode RIP to an offset from module start or init start now. tested on linux->next (tag next-20211220) Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com> --- kernel/module.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)