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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Christophe Leroy wrote:
> 
> 
> On 04/24/2020 06:26 PM, Naveen N. Rao wrote:
>> Steven Rostedt wrote:
>>> On Thu, 23 Apr 2020 17:41:52 +0200
>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>> > 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 ?
>> 
>> Thanks for the review.
>> 
>> I noticed this as a warning from checkpatch.pl, but this looked compact 
>> and correct for use in the two following functions. You'll notice that I 
>> added it just before the two functions this is used in.
>> 
>> I suppose 'goto err' is usable too, but the ftrace code (patch 2) will 
>> end up with more changes. I'm also struggling to see how a 'goto' is 
>> less offensive. I think Steve's suggestion below would be the better way 
>> to go, to make things explicit.
>> 
> 
> Sure it's be more explicit, but then more lines also. 3 lines for only 
> one really usefull.
> 
> With goto, I would look like:
> 
> diff --git a/arch/powerpc/kernel/optprobes.c 
> b/arch/powerpc/kernel/optprobes.c
> index 046485bb0a52..938208f824da 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct 
> optimized_kprobe *op)
>   	}
>   }
> 
> -#define PATCH_INSN(addr, instr)						     \
> +#define PATCH_INSN(addr, instr, label)						     \
>   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;						     \
> +		goto label;						     \
>   	}								     \
>   } while (0)

My earlier complaint was that this would still add a flow control 
statement, so didn't look to immediately address your original concern.  
However, I suppose introduction of an explicit label makes things a bit 
better.

In addition:

<snip>
> @@ -291,23 +297,8 @@ int arch_prepare_optimized_kprobe(struct 
> optimized_kprobe *op, struct kprobe *p)
>   		goto error;
>   	}
> 
> -	rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
> -	if (rc) {
> -		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
> -				__func__, __LINE__,
> -				(void *)(buff + TMPL_CALL_HDLR_IDX), rc);
> -		rc = -EFAULT;
> -		goto error;
> -	}
> -
> -	rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
> -	if (rc) {
> -		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
> -				__func__, __LINE__,
> -				(void *)(buff + TMPL_EMULATE_IDX), rc);
> -		rc = -EFAULT;
> -		goto error;
> -	}
> +	PATCH_INSN(buff + TMPL_CALL_HDLR_IDX, branch_op_callback, efault);
> +	PATCH_INSN(buff + TMPL_EMULATE_IDX, branch_emulate_step, efault);

I like how this variant can cover additional uses of patch_instruction() 
here.

I will use this variant. Thanks for the suggestion!


- Naveen