Re: [PATCH V4 7/9] x86/alternative: Batch of patch operations

[Steven Rostedt]

On Thu, 14 Feb 2019 13:53:30 +0100
Borislav Petkov <bp@xxxxxxxxx> wrote:

> >  arch/x86/include/asm/text-patching.h |  15 ++++
> >  arch/x86/kernel/alternative.c        | 124 ++++++++++++++++++++++-----
> >  2 files changed, 118 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h
> > index e85ff65c43c3..42ea7846df33 100644
> > --- a/arch/x86/include/asm/text-patching.h
> > +++ b/arch/x86/include/asm/text-patching.h
> > @@ -18,6 +18,20 @@ static inline void apply_paravirt(struct paravirt_patch_site *start,
> >  #define __parainstructions_end	NULL
> >  #endif
> >  
> > +/*
> > + * Currently, the max observed size in the kernel code is
> > + * JUMP_LABEL_NOP_SIZE/RELATIVEJUMP_SIZE, which are 5.
> > + * Raise it if needed.
> > + */
> > +#define POKE_MAX_OPCODE_SIZE	5
> > +
> > +struct text_to_poke {  
> 
> That name needs bikeshedding. Maybe struct patch_loc is more fitting as
> it is a patch location descriptor, AFAICT.

Although you did say above that "patch" is an overloaded term. Also, I
do find the current name rather descriptive.

> 
> > +	void *handler;  
> 
> Oh my, took me a while to realize that this is not really a function
> which is supposed to handle something but the place we go to when the
> temporary breakpoint hits. Please change that name and all the code
> which calls it handler. It is very misleading.

	void *trampoline;

?

> 
> > +	void *addr;
> > +	size_t len;
> > +	const char opcode[POKE_MAX_OPCODE_SIZE];
> > +};
> > +
> >  extern void *text_poke_early(void *addr, const void *opcode, size_t len);
> >  
> >  /*
> > @@ -37,6 +51,7 @@ extern void *text_poke_early(void *addr, const void *opcode, size_t len);
> >  extern void *text_poke(void *addr, const void *opcode, size_t len);
> >  extern int poke_int3_handler(struct pt_regs *regs);
> >  extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
> > +extern void text_poke_bp_batch(struct text_to_poke *tp, unsigned int nr_entries);
> >  extern int after_bootmem;
> >  
> >  #endif /* _ASM_X86_TEXT_PATCHING_H */
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 202af29c43c0..318b6867dc5a 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/stop_machine.h>
> >  #include <linux/slab.h>
> >  #include <linux/kdebug.h>
> > +#include <linux/kprobes.h>
> > +#include <linux/bsearch.h>
> >  #include <asm/text-patching.h>
> >  #include <asm/alternative.h>
> >  #include <asm/sections.h>
> > @@ -738,10 +740,26 @@ static void do_sync_core(void *info)
> >  }
> >  
> >  static bool bp_patching_in_progress;
> > -static void *bp_int3_handler, *bp_int3_addr;
> > +static struct text_to_poke *bp_int3_tpv;
> > +static unsigned int bp_int3_tpv_nr;  
> 
> Those names need bikeshedding. See below.

But they are named after you! "bp_" :-)

-- Steve

> 
> > +
> > +static int text_bp_batch_bsearch(const void *key, const void *elt)  
>