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

[Steven Rostedt]

On Thu, 23 Apr 2020 17:41:52 +0200
Christophe Leroy <christophe.le...@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 ?

#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.

-- Steve