Date:   Fri, 3 Feb 2023 08:59:21 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Jeff Xie <xiehuan09@gmail.com>
Cc:     Tiezhu Yang <yangtiezhu@loongson.cn>,
        Huacai Chen <chenhuacai@kernel.org>,
        WANG Xuerui <kernel@xen0n.name>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 0/5] Add kprobe and kretprobe support for LoongArch
Message-Id: <20230203085921.7f8e58c89075d708c266f730@kernel.org>
In-Reply-To: <CAEr6+EBYF2xqZWEuZaz5un5FF3Jb-rSAQp3s3uojsovm9RcUYA@mail.gmail.com>
References: <1674007261-9198-1-git-send-email-yangtiezhu@loongson.cn>
        <CAAhV-H4aTd6_cSy45KKjv-KrLTiwT4iG6+fkb84KfCrL3Y+hpg@mail.gmail.com>
        <CAAhV-H5WN5E=0Z9wpbXDc6VO7Nc+j7PGvnyAAGOmCRMJkdwSYw@mail.gmail.com>
        <CAEr6+ECO-=jfhzHrcdKGx0MsjMBMiN6wsBPCfv7CaXo_amAWWg@mail.gmail.com>
        <02806f85-bc09-d316-f058-3947353cb190@loongson.cn>
        <CAEr6+EBYF2xqZWEuZaz5un5FF3Jb-rSAQp3s3uojsovm9RcUYA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2 Feb 2023 11:33:23 +0800
Jeff Xie <xiehuan09@gmail.com> wrote:

> On Thu, Feb 2, 2023 at 10:23 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> >
> >
> >
> > On 02/01/2023 05:40 PM, Jeff Xie wrote:
> > > On Wed, Feb 1, 2023 at 12:56 PM Huacai Chen <chenhuacai@kernel.org> wrote:
> > >>
> > >> Hi, Jeff,
> > >>
> > >> Could you please pay some time to test this series? Thank you.
> > >
> > > Thanks for notifying me about the test.
> > >
> > > I have tested the patchset(based on the
> > > https://github.com/loongson/linux/tree/loongarch-next),
> > > I found that some functions can't  be probed e.g. scheduler_tick() or
> > > uart_write_wakeup()
> > > the two functions have the same point,  they are all run in the hardirq context.
> > >
> > > I don't know if it's related to the hardirq context, I haven't had
> > > time to study this patchset carefully.
> > > and they can be probed in the x86_64 arch.
> > >
> > > root@loongarch modules]# insmod ./kprobe_example.ko symbol=scheduler_tick
> > > insmod: can't insert './kprobe_example.ko': invalid parameter
> > >
> > > dmesg:
> > > [   39.806435] kprobe_init: register_kprobe failed, returned -22
> > >
> >
> > Thanks for your test.
> >
> > On my test environment, I can not reproduce the above issue,
> > here are the test results, it seems no problem.
> >
> > [root@linux loongson]# dmesg -c
> > [root@linux loongson]# uname -m
> > loongarch64
> > [root@linux loongson]# modprobe kprobe_example symbol=scheduler_tick
> > [root@linux loongson]# rmmod kprobe_example
> > [root@linux loongson]# dmesg | tail -2
> > [ 3317.138086] handler_post: <scheduler_tick> p->addr =
> > 0x0000000065d12f66, estat = 0xc0000
> > [ 3317.154086] kprobe_exit: kprobe at 0000000065d12f66 unregistered
> >
> > [root@linux loongson]# dmesg -c
> > [root@linux loongson]# uname -m
> > loongarch64
> > [root@linux loongson]# modprobe kprobe_example symbol=uart_write_wakeup
> > [root@linux loongson]# rmmod kprobe_example
> > [root@linux loongson]# dmesg | tail -2
> > [ 3433.502092] handler_post: <uart_write_wakeup> p->addr =
> > 0x0000000019718061, estat = 0xc0000
> > [ 3433.762085] kprobe_exit: kprobe at 0000000019718061 unregistered
> >
> > Additionally, "register_kprobe failed, returned -22" means the symbol
> > can not be probed, here is the related code:
> >
> > register_kprobe()
> >    check_kprobe_address_safe()
> >
> > static int check_kprobe_address_safe(struct kprobe *p,
> >                                      struct module **probed_mod)
> > {
> >         int ret;
> >
> >         ret = check_ftrace_location(p);
> >         if (ret)
> >                 return ret;
> >         jump_label_lock();
> >         preempt_disable();
> >
> >         /* Ensure it is not in reserved area nor out of text */
> >         if (!(core_kernel_text((unsigned long) p->addr) ||
> >             is_module_text_address((unsigned long) p->addr)) ||
> >             in_gate_area_no_mm((unsigned long) p->addr) ||
> >             within_kprobe_blacklist((unsigned long) p->addr) ||
> >             jump_label_text_reserved(p->addr, p->addr) ||
> >             static_call_text_reserved(p->addr, p->addr) ||
> >             find_bug((unsigned long)p->addr)) {
> >                 ret = -EINVAL;
> >                 goto out;
> >         }
> > ...
> > }
> 
> Today I looked at the code, this has nothing to do with hardirq :-)
> because I enabled this kernel option CONFIG_DYNAMIC_FTRACE, the
> loongarch should not support the option yet.

Until you implement the feature, "HAVE_<FEATURE>" should not be selected
in arch/*/Kconfig. So if the DYNAMIC_FTRACE is not implemented,
please drop "select HAVE_DYNAMIC_FTRACE"...

Thank you,


> 
> #ifdef CONFIG_DYNAMIC_FTRACE
> unsigned long ftrace_location(unsigned long ip);
> 
> #else /* CONFIG_DYNAMIC_FTRACE */
> 
> static inline unsigned long ftrace_location(unsigned long ip)
> {
>         return 0;
> }
> 
> #endif
> 
> 
> static int check_ftrace_location(struct kprobe *p)
> {
>         unsigned long addr = (unsigned long)p->addr;
> 
>         if (ftrace_location(addr) == addr) {
> #ifdef CONFIG_KPROBES_ON_FTRACE
>                 p->flags |= KPROBE_FLAG_FTRACE;
> #else   /* !CONFIG_KPROBES_ON_FTRACE */
>                 return -EINVAL;  // get error from here
> #endif
>         }
>         return 0;
> }
> 
> static int check_kprobe_address_safe(struct kprobe *p,
>                                      struct module **probed_mod)
> {
>         int ret;
> 
>         ret = check_ftrace_location(p);
>         if (ret)
>                 return ret; //  return -EINVAL
> }
> 
> 
> >
> > Thanks,
> > Tiezhu
> >
> 
> 
> -- 
> Thanks,
> JeffXie


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>