From: Qais Yousef <qais.yousef@arm.com>
To: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Russell King <linux@armlinux.org.uk>,
linux-arm-kernel@lists.infradead.org,
Florian Fainelli <f.fainelli@gmail.com>,
linux-kernel@vger.kernel.org,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Date: Tue, 9 Mar 2021 17:42:01 +0000 [thread overview]
Message-ID: <20210309174201.n53za7mw33dqyleh@e107158-lin.cambridge.arm.com> (raw)
In-Reply-To: <0c122390-6e76-f773-86e9-8c085f4384f2@nokia.com>
On 03/08/21 08:58, Alexander Sverdlin wrote:
> Hi!
>
> On 07/03/2021 18:26, Qais Yousef wrote:
> > I tried on 5.12-rc2 and 5.11 but couldn't reproduce the problem using your
I still can't reproduce on 5.12-rc2.
I do have CONFIG_ARM_MODULE_PLTS=y. Do you need to do something else after
loading the module? I tried starting ftrace, but maybe there's a particular
combination required?
> > instructions on the other email. But most likely because I'm hitting another
> > problem that could be masking it. I'm not sure it is related or just randomly
> > happened to hit it.
> >
> > Did you see something similar?
>
> [...]
>
> > [ 0.000000] [<c1b01a38>] (ftrace_bug) from [<c046316c>] (ftrace_process_locs+0x2b0/0x518)
> > [ 0.000000] r7:c3817ac4 r6:c38040c0 r5:00000a3c r4:000134e4
> > [ 0.000000] [<c0462ebc>] (ftrace_process_locs) from [<c2b25240>] (ftrace_init+0xc8/0x174)
> > [ 0.000000] r10:c2ffa000 r9:c2be8a78 r8:c2c5d1fc r7:c2c0c208 r6:00000001 r5:c2d0908c
> > [ 0.000000] r4:c362f518
> > [ 0.000000] [<c2b25178>] (ftrace_init) from [<c2b00e14>] (start_kernel+0x2f4/0x5b8)
> > [ 0.000000] r9:c2be8a78 r8:dbfffec0 r7:00000000 r6:c36385cc r5:c2d08f00 r4:c2ffa000
> > [ 0.000000] [<c2b00b20>] (start_kernel) from [<00000000>] (0x0)
>
> This means, FTRACE has more problems with your kernel/compiler/platform, I've addressed similar issue
> in the past, but my patch should be long merged:
>
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1817963.html
>
> Could it be the same problem as here:
> https://www.spinics.net/lists/arm-kernel/msg854022.html
>
> Seems that the size check deserves something line BUILD_BUG_ON() with FTRACE...
So I only see this when I convert all modules to be built-in
sed -i 's/=m/=y/' .config
FWIW, I see the problem with your patch applied too. Trying to dig more into
it..
>
> >> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> >> index 9a79ef6..fa867a5 100644
> >> --- a/arch/arm/kernel/ftrace.c
> >> +++ b/arch/arm/kernel/ftrace.c
> >> @@ -70,6 +70,19 @@ int ftrace_arch_code_modify_post_process(void)
> >>
> >> static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
> >> {
> >> + s32 offset = addr - pc;
> >> + s32 blim = 0xfe000008;
> >> + s32 flim = 0x02000004;
> >
> > This look like magic numbers to me..
>
> These magic numbers are most probably the reason for your FTRACE to resign...
> Those are backward- and forward-branch limits. I didn't find the matching DEFINEs
> in the kernel, but I would be happy to learn them. I can also put some comments,
> but I actually thought the purpose would be obvious from the code...
So I did dig more into it. The range is asymmetrical indeed. And the strange
offset is to cater for the pc being incremented by +8 (+4 for thumb2).
You're duplicating the checks in __arm_gen_branch_{thumb2, arm}(). As you noted
__arm_gen_branch() which is called by arm_gen_branch_link() will end up doing
the exact same check and return 0. So why do you need to duplicate the check
here? We can do something about the WARN_ON_ONCE(1).
[...]
> >> +
> >> return arm_gen_branch_link(pc, addr);
> >> }
> >>
> >> @@ -124,10 +137,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> >> {
> >> unsigned long new, old;
> >> unsigned long ip = rec->ip;
> >> + unsigned long aaddr = adjust_address(rec, addr);
> >>
> >> old = ftrace_nop_replace(rec);
> >>
> >> - new = ftrace_call_replace(ip, adjust_address(rec, addr));
> >> + new = ftrace_call_replace(ip, aaddr);
> >> +
> >> +#ifdef CONFIG_ARM_MODULE_PLTS
> >> + if (!new) {
> >> + struct module *mod = rec->arch.mod;
> >> +
> >> + if (mod) {
> >
> > What would happen if !new and !mod?
>
> I believe, that's exactly what happens in the dump you experience with your kernel.
> This is not covered by this patch, this patch covers the issue with modules in vmalloc area.
>
> >> + aaddr = get_module_plt(mod, ip, aaddr);
> >> + new = ftrace_call_replace(ip, aaddr);
> >
> > I assume we're guaranteed to have a sensible value returned in 'new' here?
>
> Otherwise you'd see the dump you see :)
> It relies on the already existing error handling.
I understand from this there are still loose ends to be handled in this area of
the code.
I admit I need to spend more time to understand why I get the failure above and
how this overlaps with your proposal. But as it stands it seems there's more
work to be done here.
Thanks
--
Qais Yousef
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-09 17:44 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-27 11:09 [PATCH v7 0/2] ARM: Implement MODULE_PLT support in FTRACE Alexander A Sverdlin
2021-01-27 11:09 ` [PATCH v7 1/2] ARM: PLT: Move struct plt_entries definition to header Alexander A Sverdlin
2021-01-27 20:25 ` Florian Fainelli
2021-01-27 11:09 ` [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support Alexander A Sverdlin
2021-01-27 19:36 ` Florian Fainelli
2021-03-07 17:26 ` Qais Yousef
2021-03-08 7:58 ` Alexander Sverdlin
2021-03-09 17:42 ` Qais Yousef [this message]
2021-03-10 7:23 ` Alexander Sverdlin
2021-03-10 16:14 ` Florian Fainelli
2021-03-10 17:17 ` Alexander Sverdlin
2021-03-12 17:24 ` Qais Yousef
2021-03-12 18:35 ` Florian Fainelli
2021-03-14 22:02 ` Qais Yousef
2021-03-21 19:06 ` Qais Yousef
2021-03-22 15:01 ` Steven Rostedt
2021-03-22 16:32 ` Qais Yousef
2021-03-22 17:02 ` Alexander Sverdlin
2021-03-23 22:22 ` Qais Yousef
2021-03-24 3:37 ` Florian Fainelli
2021-03-24 16:10 ` Qais Yousef
2021-03-24 9:04 ` Alexander Sverdlin
2021-03-24 15:57 ` Qais Yousef
2021-03-24 16:33 ` Alexander Sverdlin
2021-03-24 16:46 ` Qais Yousef
2021-03-15 9:19 ` Alexander Sverdlin
2021-02-03 18:23 ` [PATCH v7 0/2] ARM: Implement MODULE_PLT support in FTRACE Florian Fainelli
2021-02-15 18:31 ` Ard Biesheuvel
2021-03-02 8:29 ` Linus Walleij
2021-03-02 10:00 ` Alexander Sverdlin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210309174201.n53za7mw33dqyleh@e107158-lin.cambridge.arm.com \
--to=qais.yousef@arm.com \
--cc=alexander.sverdlin@nokia.com \
--cc=ard.biesheuvel@linaro.org \
--cc=f.fainelli@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mingo@redhat.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).