Re: [PATCH v1 2/9] uretprobes: Reserve the first slot in xol_vma for trampoline

[Oleg Nesterov <oleg@redhat.com>]

On 04/07, Srikar Dronamraju wrote:
>
> * Anton Arapov <anton@redhat.com> [2013-04-03 18:00:32]:
>
> > Allocate trampoline page, as the very first one in uprobed
> > task xol area, and fill it with breakpoint opcode.
> >
> > Also introduce get_trampoline_vaddr() helper, to wrap the
> > trampoline address extraction from area->vaddr. That removes
> > confusion and eases the debug experience in case ->vaddr
> > notion will be changed.
> >
> > v1 changes:
> > * rework get_trampoline_vaddr() helper.
> > * init xol_area->slot_count.
> >
> > Signed-off-by: Anton Arapov <anton@redhat.com>
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

> > +static unsigned long get_trampoline_vaddr(void)
> > +{
> > +	struct xol_area *area;
> > +	unsigned long trampoline_vaddr = -1;
> > +
> > +	area = current->mm->uprobes_state.xol_area;
> > +	smp_read_barrier_depends();
>
> check: do we need this barrier?

In theory yes. For example, we do not want the false positive in
handle_swbp() which does

	if (bp_vaddr == get_trampoline_vaddr())
		handler_uretprobe();

In theory we can race with another thread which initializes area
(in particular area->vaddr) and then sets uprobes_state.xol_area = area
in xol_add_vma().

If we see ->xol_area != NULL we must ensure that we read the correct
value of area->vaddr, so we need a barrier.

In short. Please note get_xol_area()->smp_read_barrier_depends(),
get_trampoline_vaddr() needs it for the same reason.

Oleg.