linux-kernel - Re: [PATCH 5/5] uprobes/x86: Move default_xol_ops's data into arch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1398382247.4536.12.camel@oc7886638347.ibm.com.usor.ibm.com>
Date:	Thu, 24 Apr 2014 16:30:47 -0700
From:	Jim Keniston <jkenisto@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <aarapov@...hat.com>,
	David Long <dave.long@...aro.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Jonathan Lebon <jlebon@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] uprobes/x86: Move default_xol_ops's data into
 arch_uprobe->def

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@...hat.com>

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@...ibm.com>

> ---
>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/