Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Sep 2022 11:32:20 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Binglei Wang <wang.binglei@h3c.com>
CC:     <paul.walmsley@sifive.com>, <palmer@dabbelt.com>,
        <aou@eecs.berkeley.edu>, <linux-riscv@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <l3b2w1@gmail.com>
Subject: Re: [PATCH] rethook: add riscv rethook implementation.
Message-ID: <YymWtBokuXK3cGEa@wendy>
References: <20220920093630.32085-1-wang.binglei@h3c.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220920093630.32085-1-wang.binglei@h3c.com>
Precedence: bulk

On Tue, Sep 20, 2022 at 05:36:30PM +0800, Binglei Wang wrote:
> From: "wang.binglei" <wang.binglei@h3c.com>
> 
> Most of the code copied from
> arch/riscv/kernel/probes/kprobes_trampoline.S

Hey Wang Binglei,

Please use the commit log to explain the reasons behind the change you
are making:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

> 
> Signed-off-by: wang.binglei <wang.binglei@h3c.com>

Unfortunately I don't know much about Asian naming, but I assume that
the . is not part of your name?

> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index e6e950b7c..2c1847921 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -345,6 +345,7 @@ int __init arch_populate_kprobe_blacklist(void)
>         return ret;
>  }
> 
> +#ifndef CONFIG_KRETPROBE_ON_RETHOOK

This seems quite unusual, other archs don't seem to have ifdef-ery
using CONFIG_KRETPROBE_ON_RETHOOK in their arch code so why should
RISC-V?

>  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
>  {
>         return (void *)kretprobe_trampoline_handler(regs, NULL);
> @@ -357,6 +358,12 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>         ri->fp = NULL;
>         regs->ra = (unsigned long) &__kretprobe_trampoline;
>  }
> +#else
> +void __kprobes *trampoline_probe_handler(struct pt_regs *regs)
> +{
> +       return NULL;
> +}
> +#endif
 

> diff --git a/arch/riscv/kernel/probes/rethook_trampoline.S b/arch/riscv/kernel/probes/rethook_trampoline.S
> new file mode 100644
> index 000000000..aa79630ac
> --- /dev/null
> +++ b/arch/riscv/kernel/probes/rethook_trampoline.S
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * rethook trampoline.
> + * Copied from arch/riscv/kernel/probes/kprobes_trampoline.S

Is this a 1:1 copy? If so, could the code be shared?
 
> This e-mail and its attachments contain confidential information from New H3C, which is
> intended only for the person or entity whose address is listed above. Any use of the
> information contained herein in any way (including, but not limited to, total or partial
> disclosure, reproduction, or dissemination) by persons other than the intended
> recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
> by phone or email immediately and delete it!

Uh-oh! You'll have to work with your IT to get this removed before your
patches can be accepted:
https://lore.kernel.org/all/YgEnxmD9ZE4jVhP5@kroah.com/

The patch does not apply to -next for me either..

Thanks,
Conor.