[PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
Naveen N. Rao
naveen.n.rao at linux.vnet.ibm.com
Sat Apr 25 04:26:25 AEST 2020
Steven Rostedt wrote:
> On Thu, 23 Apr 2020 17:41:52 +0200
> Christophe Leroy <christophe.leroy at 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
More information about the Linuxppc-dev
mailing list