Re: [PATCH][RFC] jump_labels/x86: Use either 5 byte or 2 byte jumps

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 2011-10-07 at 12:40 -0700, Jeremy Fitzhardinge wrote:
> On 10/07/2011 10:09 AM, Steven Rostedt wrote:
> > diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> > index 3fee346..1f7f88f 100644
> > --- a/arch/x86/kernel/jump_label.c
> > +++ b/arch/x86/kernel/jump_label.c
> > @@ -16,34 +16,75 @@
> >  
> >  #ifdef HAVE_JUMP_LABEL
> >  
> > +static unsigned char nop_short[] = { P6_NOP2 };
> > +
> >  union jump_code_union {
> >  	char code[JUMP_LABEL_NOP_SIZE];
> >  	struct {
> >  		char jump;
> >  		int offset;
> >  	} __attribute__((packed));
> > +	struct {
> > +		char jump_short;
> > +		char offset_short;
> > +	} __attribute__((packed));
> >  };
> >  
> >  void arch_jump_label_transform(struct jump_entry *entry,
> >  			       enum jump_label_type type)
> >  {
> >  	union jump_code_union code;
> > +	unsigned char op;
> > +	unsigned size;
> > +	unsigned char nop;
> > +
> > +	/* Use probe_kernel_read()? */
> > +	op = *(unsigned char *)entry->code;
> > +	nop = ideal_nops[NOP_ATOMIC5][0];
> >  
> >  	if (type == JUMP_LABEL_ENABLE) {
> > -		code.jump = 0xe9;
> > -		code.offset = entry->target -
> > -				(entry->code + JUMP_LABEL_NOP_SIZE);
> > -	} else
> > -		memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
> > +		if (op == 0xe9 || op == 0xeb)
> > +			/* Already enabled. Warn? */
> > +			return;
> > +
> > +		/* FIXME for all archs */
> 
> By "archs", do you mean different x86 variants?

Yeah, that was a confusing use of archs. This was to make sure it works
for all nops for different variants of x86.

> 
> > +		if (op == nop_short[0]) {
> 
> My gut feeling is that all this "trying to determine the jump size by
> sniffing the instruction" stuff seems pretty fragile.  Couldn't you
> store the jump size in the jump_label structure (even as a bit hidden
> away somewhere)?

We could but it's not as fragile as you think. This is machine code, and
it should be a jump or not. I could add more checks, that is, to look at
the full nop to make sure it is truly a nop. But for the jump side, a
byte instruction that starts with e9 is definitely a jump.

I could harden this more like what we do with mcount updates in the
function tracer. I actually calculate what I expect to be there before
looking at what is there. The entire instruction is checked. If it does
not match, then we fail and give big warnings about it.

Other than that, it should be quite solid. If we don't get a match, we
should warn and disable jump labels.

No BUG()!

-- Steve