Re: [PATCHv11 2.6.36-rc2-tip 3/15] 3: uprobes: Slot allocationfor Execution out of line(XOL)

[Srikar Dronamraju]

> > +struct uprobes_xol_area {
> > +	spinlock_t lock;	/* protects bitmap and slot (de)allocation*/
> > +	unsigned long *bitmap;	/* 0 = free slot */
> 
> Since you have a static sized bitmap, why not simply declare it here?
> 
> 	DECLARE_BITMAP(bitmap, MAX_UPROBES_XOL_SLOTS;

Okay,  will do.

> 
> > +	/*
> > +	 * We keep the vma's vm_start rather than a pointer to the vma
> > +	 * itself.  The probed process or a naughty kernel module could make
> > +	 * the vma go away, and we must handle that reasonably gracefully.
> > +	 */
> 
> Naughty kernel modules we don't care about, but yeah, it appears vma's
> installed using install_special_mapping() can be unmapped by the process
> itself,.. curious. 

Will clean this up.
> 
> Anyway, you could install your own vm_ops and provide a close method to
> track this.
> 
> > +	unsigned long vaddr;		/* Page(s) of instruction slots */
> > +};
> > +

<snipped>

> > +	vma = find_vma(mm, addr);
> > +
> > +	/* Don't expand vma on mremap(). */
> > +	vma->vm_flags |= VM_DONTEXPAND | VM_DONTCOPY;
> > +	area->vaddr = vma->vm_start;
> 
> Seems interesting,.. why not use install_special_mapping(), that's what
> the VDSO uses.

Okay, I hadnt looked at install_special_mapping earlier so I will take a
look and incorporate it. However I am not clear at this point what
install_special_mapping is giving us here.  Also install_special_mapping
is already defining its own vm_ops esp a close method thats doesnt seem
to be doing anything. So at this point I am not clear how we are link
the vm_ops, the close method and install_special_mapping.

> 
> > +/*
> > + * xol_alloc_area - Allocate process's uprobes_xol_area.
> > + * This area will be used for storing instructions for execution out of
> > + * line.
> 
> It doesn't actually do that, xol_add_vma() does that, this allocates the
> storage management bits.

Okay, will clean up. 

> 
> > + * Called with mm->uproc->mutex locked.
> 
> There's a nice way to not have to write that:
> 
>   lockdep_assert_held(&mm->uproc->mutex);

Okay, will do.

> 
> I would call that allocate, find would imply a constant operation, but
> you actually change the state.
> 

Okay, 

> > + * Called when holding xol_area->lock
> 
>   lockdep_assert_held(&area->lock);
> 
> > + */
> > +static unsigned long xol_take_insn_slot(struct uprobes_xol_area *area)
> > +{
> > +	unsigned long slot_addr;
> > +	int slot_nr;
> > +
> > +	slot_nr = find_first_zero_bit(area->bitmap, UINSNS_PER_PAGE);
> > +	if (slot_nr < UINSNS_PER_PAGE) {
> > +		set_bit(slot_nr, area->bitmap);
> 
> Since its all serialized by xol_area->lock, why use an atomic bitop?

Okay will use the non-atomic set_bit.

> 
> > +		slot_addr = area->vaddr +
> > +				(slot_nr * UPROBES_XOL_SLOT_BYTES);
> > +		return slot_addr;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

<snipped>

> > +	/*
> > +	 * Initialize the slot if user_bkpt->vaddr points to valid
> > +	 * instruction slot.
> > +	 */
> > +	if (likely(xol_vaddr) && user_bkpt->vaddr) {
> 
> if (!xol_vaddr)
>   goto bail;
> 
> gives nices code, and saves an indent level.
> 
> Also, why would we ever get here with !user_bkpt->vaddr.

For now, user_bkpt->vaddr will always be set when we are here. 
However when we add uretprobe support, we would then get here with
user_bkpt->vaddr being NULL. 

I would drop the check for now, but add it later when we add the return
probe support.

> 
> (fwiw, my fingers hate bkpt, they either want to type bp, or brkpt)
> 

I had renamed the structure from ubp to user_bkpt based on your
comments. I had actually mentioned this in the summary mail that I had
sent on Jan 22 this year. I am fine to rename it to user_bp if that
helps but if you feel ubp is better, I can revert to ubp too.


> > +		len = access_process_vm(current, xol_vaddr, user_bkpt->insn,
> > +						UPROBES_XOL_SLOT_BYTES, 1);
> > +		if (unlikely(len < UPROBES_XOL_SLOT_BYTES))
> > +			printk(KERN_ERR "Failed to copy instruction at %#lx "
> > +					"len = %d\n", user_bkpt->vaddr, len);
> > +	}
> > +
> > +	/*
> > +	 * Update user_bkpt->xol_vaddr after giving a chance for the slot to
> > +	 * be initialized.
> > +	 */
> > +	mb();
> 
> Where is the matching barrier?

I dont want the compiler to reorder the instructions and do the
assignment for user_bkpt to be done before we complete the copy above.

If the assignment happens before we copy the content into the slot,
someother thread that might hit the same probe actually things the slot
is ready and tries to jump to that slot even before the slot is
initialized.

Please let me know if I could have done it differently.

> 
> > +	user_bkpt->xol_vaddr = xol_vaddr;
> > +	return user_bkpt->xol_vaddr;
> > +}
> > +
<snipped>

> 
> > +		spin_unlock_irqrestore(&xol_area->lock, flags);
> > +		found = 1;
> > +	}
> > +
> > +	if (!found)
> > +		printk(KERN_ERR "%s: no XOL vma for slot address %#lx\n",
> > +						__func__, slot_addr);
> 
> funny code flow,.. s/found = 1/return/ and loose the conditional and
> indent?

Okay, will do.

> 
> > +static int xol_validate_vaddr(struct pid *pid, unsigned long vaddr,
> > +				struct uprobes_xol_area *xol_area)
> > +{
> > +	struct task_struct *tsk;
> > +	unsigned long vma_end;
> > +	int result;
> > +
> > +	if (unlikely(!xol_area))
> > +		return 0;
> > +
> > +	tsk = get_pid_task(pid, PIDTYPE_PID);
> > +	if (unlikely(!tsk))
> > +		return -EINVAL;
> > +
> > +	result = validate_address(tsk, vaddr);
> > +	if (result != 0)
> > +		goto validate_end;
> > +
> > +	vma_end = xol_area->vaddr + PAGE_SIZE;
> > +	if (xol_area->vaddr <= vaddr && vaddr < vma_end)
> > +		result = 1;
> > +
> > +validate_end:
> > +	put_task_struct(tsk);
> > +	return result;
> > +}
> 
> This doesn't actually appear used in this patch,.. does it want to live
> elsewhere?

Yes, xol_validate_vaddr gets used in the next patch.  So probably it can
be moved to the next patch.

--
Thanks and Regards
Srikar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/