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

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

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.

> 
> #define PATCH_INSN(addr, instr) \
> ({
> 	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);	     \
> 	rc;								     \
> })
> 
> 
> Then you can just do:
> 
> 	ret = PATCH_INSN(...);
> 	if (ret)
> 		return ret;
> 
> in the code.

That's really nice. However, in this case, I guess I can simply use an 
inline function? The primary reason I used the macro was for including a 
'return' statement in it.


Thanks for the review!
- Naveen