[PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()

[Christophe Leroy]

Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> patch_instruction() can fail in some scenarios. Add appropriate error
> checking so that such failures are caught and logged, and suitable error
> code is returned.
> 
> Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to patch_instruction()")
> Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao at linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/kprobes.c   | 10 +++-
>   arch/powerpc/kernel/optprobes.c | 99 ++++++++++++++++++++++++++-------
>   2 files changed, 87 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 81efb605113e..4a297ae2bd87 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
>   
>   void arch_arm_kprobe(struct kprobe *p)
>   {
> -	patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
> +	int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
> +
> +	if (rc)
> +		WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr, rc);
>   }
>   NOKPROBE_SYMBOL(arch_arm_kprobe);
>   
>   void arch_disarm_kprobe(struct kprobe *p)
>   {
> -	patch_instruction(p->addr, p->opcode);
> +	int rc = patch_instruction(p->addr, p->opcode);
> +
> +	if (rc)
> +		WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr, rc);
>   }
>   NOKPROBE_SYMBOL(arch_disarm_kprobe);
>   
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 024f7aad1952..046485bb0a52 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
>   	}
>   }
>   
> +#define PATCH_INSN(addr, instr)						     \
> +do {									     \
> +	int rc = patch_instruction((unsigned int *)(addr), instr);	     \
> +	if (rc) {							     \
> +		pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
> +				__func__, __LINE__,			     \
> +				(void *)(addr), (void *)(addr), rc);	     \
> +		return rc;						     \
> +	}								     \
> +} while (0)
> +

I hate this kind of macro which hides the "return".

What about keeping the return action in the caller ?

Otherwise, what about implementing something based on the use of goto, 
on the same model as unsafe_put_user() for instance ?


Christophe