linux-kernel - Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup

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]

Date:	Mon, 11 Nov 2013 17:55:43 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Ingo Molnar <mingo@...e.hu>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

On 11/11, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@...hat.com> [2013-11-08 20:00:03]:
>
> > uprobe_task->vaddr is a bit strange. First of all it is not really
> > needed, we can move it into arch_uprobe_task. The generic code uses
> > it only to pass the additional argument to arch_uprobe_pre_xol(),
> > and since it is always equal to instruction_pointer() this looks
> > even more strange.
> >
>
> While the code changes look good, I would disagree with the above
> statement.  uprobe_task->vaddr is a bit strange only in
> uprobe_copy_process() and not in arch_uprobe_pre_xol() context.
> uprobe_task->vaddr was used to refer to the actual instruction pointer

Yes, and it is always equal to regs->ip when pre_ssout() is called,

> and do the necessary fixups after single stepping out of line.

Exactly. So it is write-only (and meaningless) to the generic uprobe
code. We can (and perhaps should) move it into autask->saved_vaddr,
arch_uprobe_pre_xol() can initialize it.

> The casual reading of this commit message, one can get an impression
> that vaddr is never needed.

See above. The changelog doesn't say we can simply remove it, it says
"move it".

> Your change still retains it.

Of course we can't kill utas->vaddr until we change x86/powerpc. And
just in case, of course I understand that this is minor and even
subjective.

But at least this patch logically "joins" autask and vaddr, and I
believe this is good because they are always used together, because,
well, logically vaddr belongs to autask ;)

> So can we
> modify the commit message accordingly?

Well, I'll try... but perhaps you can help? I mean, I am not sure
about how I can improve it. Could you suggest a better wording?

> Otherwise
> Acked-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>

Thanks!

> > @@ -72,14 +73,24 @@ enum uprobe_task_state {
> >   */
> >  struct uprobe_task {
> >  	enum uprobe_task_state		state;
> > -	struct arch_uprobe_task		autask;
> >
> > -	struct return_instance		*return_instances;
> > -	unsigned int			depth;
> > -	struct uprobe			*active_uprobe;
> > +	union {
> > +		struct {
> > +			struct arch_uprobe_task	autask;
> > +			unsigned long		vaddr;
> > +		};
> > +
> > +		struct {
> > +			struct callback_head	dup_work;
> > +			unsigned long		dup_addr;
>
> Nit:
> Can we rename dup_addr to mean that it refers to the xol; something like
> dup_xol_addr or even xol_addr. So that its more clear what address it
> refers to.

OK. How about dup_xol_work/dup_xol_vaddr ?

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/