linux-kernel - Re: [PATCHv4] uprobes: simplify rip-relative handling

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] [day] [month] [year] [list]

Date:	Thu, 01 May 2014 16:35:23 -0700
From:	Jim Keniston <jkenisto@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Denys Vlasenko <dvlasenk@...hat.com>, linux-kernel@...r.kernel.org,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCHv4] uprobes: simplify rip-relative handling

On Thu, 2014-05-01 at 20:50 +0200, Oleg Nesterov wrote:
> Thanks, I hope that Jim's ack still applies to this version.

Yes.  v4 looks fine, although Oleg has a good point about moving the
comment.  But I'd move more of the comment, starting with
 * We have to fix things up as follows:

Reviewed-by: Jim Keniston <jkenisto@...ibm.com>


> On 05/01, Denys Vlasenko wrote:
> >
> > v4: Changed arch_uprobe_xol_was_trapped() comment to reflect new logic.
> 
> Hmm. I guess you meant arch_uprobe_post_xol()... please see below.
> 
> >  static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> >  {
> >  	struct uprobe_task *utask = current->utask;
> > -	long correction = (long)(utask->vaddr - utask->xol_vaddr);
> >  
> > -	riprel_post_xol(auprobe, regs, &correction);
> > +	riprel_post_xol(auprobe, regs);
> >  	if (auprobe->def.fixups & UPROBE_FIX_IP) {
> > +		long correction = (long)(utask->vaddr - utask->xol_vaddr);
> 
> Can't resist, I'll remove this pointless cast ;)
> 
> >   * If the original instruction was a rip-relative instruction such as
> >   * "movl %edx,0xnnnn(%rip)", we have instead executed an equivalent
> > - * instruction using a scratch register -- e.g., "movl %edx,(%rax)".
> > - * We need to restore the contents of the scratch register and adjust
> > - * the ip, keeping in mind that the instruction we executed is 4 bytes
> > - * shorter than the original instruction (since we squeezed out the offset
> > - * field).  (FIX_RIP_AX or FIX_RIP_CX)
> > + * instruction using a scratch register -- e.g., "movl %edx,0xnnnn(%rax)".
> > + * We need to restore the contents of the scratch register
> > + * (FIX_RIP_AX or FIX_RIP_CX).
> >   */
> >  int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> 
> Perhaps it makes sense to move this part of the comment above
> default_post_xol_op() which actually does this...
> 
> I won't insist, I do not really care because I almost never read the comments
> anyway ;)
> 
> 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/