Re: [PATCH] rethook: add riscv rethook implementation.

[Conor Dooley]

On Tue, Sep 20, 2022 at 05:36:30PM +0800, Binglei Wang wrote:
> From: "wang.binglei" <wang.binglei@xxxxxxx>
> 
> 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@xxxxxxx>

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@xxxxxxxxx/

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

Thanks,
Conor.