Re: [PATCH 5/5] uprobes/x86: Move default_xol_ops's data into arch_uprobe->def

[Jim Keniston]

On Tue, 2014-04-22 at 16:47 +0200, Oleg Nesterov wrote:
> Finally we can move arch_uprobe->fixups/rip_rela_target_address
> into the new "def" struct and place this struct in the union, they
> are only used by default_xol_ops paths.
> 
> The patch also renames rip_rela_target_address to riprel_target just
> to make this name shorter.
> 
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>

I see a couple of nits in this patch (see below), but the others look
good.

Patches 1-5 of this set:
Reviewed-by: Jim Keniston <jkenisto@xxxxxxxxxx>

> ---
>  arch/x86/include/asm/uprobes.h |   12 ++++++----
>  arch/x86/kernel/uprobes.c      |   41 +++++++++++++++++++--------------------
>  2 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 93bee7b..72caff7 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -41,18 +41,20 @@ struct arch_uprobe {
>  		u8			ixol[MAX_UINSN_BYTES];
>  	};
> 
> -	u16				fixups;
>  	const struct uprobe_xol_ops	*ops;
> 
>  	union {
> -#ifdef CONFIG_X86_64
> -		unsigned long			rip_rela_target_address;
> -#endif
>  		struct {
>  			s32	offs;
>  			u8	ilen;
>  			u8	opc1;
> -		}				branch;
> +		}			branch;
> +		struct {
> +#ifdef CONFIG_X86_64
> +			long	riprel_target;
> +#endif
> +			u16	fixups;
> +		} 			def;

"def" is kind of ambiguous.  How about "dfault" or some such?

>  	};
>  };
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index e6314a2..69b2d61 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
...
> @@ -636,12 +635,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> 
>  	/*
>  	 * Figure out which fixups arch_uprobe_post_xol() will need to perform,
> -	 * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups
> -	 * is either zero or it reflects rip-related fixups.
> +	 * and annotate def->fixups accordingly. To start with, ->fixups is
> +	 * either zero or it reflects rip-related fixups.

That sentence stopped being true a couple of patch sets ago.
handle_riprel_insn() is called later in this function now.

>  	 */
>  	switch (OPCODE1(&insn)) {
>  	case 0x9d:		/* popf */
> -		auprobe->fixups |= UPROBE_FIX_SETF;
> +		auprobe->def.fixups |= UPROBE_FIX_SETF;
>  		break;
>  	case 0xc3:		/* ret or lret -- ip is correct */
>  	case 0xcb:
> @@ -669,9 +668,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	}
> 
>  	if (fix_ip)
> -		auprobe->fixups |= UPROBE_FIX_IP;
> +		auprobe->def.fixups |= UPROBE_FIX_IP;
>  	if (fix_call)
> -		auprobe->fixups |= UPROBE_FIX_CALL;
> +		auprobe->def.fixups |= UPROBE_FIX_CALL;
> 
>  	auprobe->ops = &default_xol_ops;
>  	return 0;

Jim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/