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: <20140425195340.GA12875@redhat.com>
Date:	Fri, 25 Apr 2014 21:53:40 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jim Keniston <jkenisto@...ux.vnet.ibm.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 04/24, Jim Keniston wrote:
>
> 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>

Thanks!

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

Heh. I am shy to admit that my plan was to name it "default". I changed
its name only after gcc told me I should learn "C".

> How about "dfault" or some such?

Then probably "dflt", this looks more consintent and more IBMish ;)

On a serious note, I agree with any naming... but "dfault" looks like
"d" fault to me. Perhaps something else?

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

Yes, but the comment mentions arch_uprobe_post_xol? Anyway, I'll update
it to say "default_post_xol_op" instead.

Or I misunderstood you ?

Oleg.

--
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/