0: Uprobes Patches

+struct user_bkpt_arch_info *arch = &user_bkpt_arch_info;

kernel/uprobes.c new file +14 -0

+unsigned long uprobes_read_vm(struct task_struct *tsk, void __user *vaddr,
+					void *kbuf, unsigned long nbytes)
+{
+	if (tsk == current) {
+		unsigned long nleft = copy_from_user(kbuf, vaddr, nbytes);
+		return nbytes - nleft;
+	} else
+		return access_process_vm(tsk, (unsigned long) vaddr, kbuf,
+							nbytes, 0);
+}
+
 
+unsigned long uprobes_write_data(struct task_struct *tsk,
+				void __user *vaddr, const void *kbuf,
+				unsigned long nbytes)

kernel/uprobes.c new file +20 -0

+static int write_opcode(struct task_struct *tsk, unsigned long vaddr,
+						user_bkpt_opcode_t opcode)
+{
+	struct mm_struct *mm;
+	struct vm_area_struct *vma;
+	struct page *old_page, *new_page;
+	void *vaddr_old, *vaddr_new;
+	pte_t orig_pte;
+	int ret = -EINVAL;
+
+	if (!tsk)
+		return ret;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return ret;
+
+	down_read(&mm->mmap_sem);
+
+	/* Read the page with vaddr into memory */

kernel/uprobes.c new file +6 -0

+	/* Allocate a page */
+	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
+	if (!new_page) {
+		ret = -ENOMEM;
+		goto put_out;
+	}

kernel/uprobes.c new file +14 -0

+			return -EINVAL;
+	}
+	return write_opcode(tsk, user_bkpt->vaddr, user_bkpt->opcode);
+}
 
+/**
+ * check_vma - verify if the address is in a executable vma.
+ * @tsk: the probed task
+ * @vaddr: virtual address of the instruction to be verified.
+ *
+ * Return 0 if vaddr is in an executable VM area,
+ * or -EINVAL otherwise.
+ */
+static int check_vma(struct task_struct *tsk, unsigned long vaddr)
+{

+/*
+ * __insert_bkpt - insert breakpoint
+ * Insert a breakpoint into the process that includes @tsk, at the
+ * virtual address @user_bkpt->vaddr.
+ *
+ * All threads of the probed process must be stopped while
+ * @__insert_bkpt() runs.

+ * Possible errors:
+ * -%ENOSYS: user_bkpt not supported for this architecture
+ * -%EINVAL: invalid instruction address
+ * -%EEXIST: breakpoint instruction already exists at that address
+ * -%EPERM: cannot probe this instruction
+ * -%EFAULT: failed to insert breakpoint instruction
+ */

kernel/uprobes.c new file +2 -0

+static int pre_sstep(struct task_struct *tsk, struct user_bkpt *user_bkpt,
+		struct user_bkpt_task_arch_info *tskinfo, struct pt_regs *regs)

kernel/uprobes.c +39 -0

+	unsigned long vaddr;		/* Page(s) of instruction slots */
+};
+
+static int xol_add_vma(struct uprobes_xol_area *area)
+{
+	struct vm_area_struct *vma;
+	struct mm_struct *mm;
+	struct file *file;
+	unsigned long addr;
+
+	mm = get_task_mm(current);
+	if (!mm)
+		return -ESRCH;
+
+	down_write(&mm->mmap_sem);
+	/*
+	 * Find the end of the top mapping and skip a page.
+	 * If there is no space for PAGE_SIZE above
+	 * that, mmap will ignore our address hint.
+	 *
+	 * We allocate a "fake" unlinked shmem file because
+	 * anonymous memory might not be granted execute
+	 * permission when the selinux security hooks have
+	 * their way.
+	 */
+	vma = rb_entry(rb_last(&mm->mm_rb), struct vm_area_struct, vm_rb);
+	addr = vma->vm_end + PAGE_SIZE;
+	file = shmem_file_setup("uprobes/xol", PAGE_SIZE, VM_NORESERVE);
+	if (!file) {
+		printk(KERN_ERR "uprobes_xol failed to setup shmem_file "
+			"while allocating vma for pid/tgid %d/%d for "
+			"single-stepping out of line.\n",
+			current->pid, current->tgid);
+		goto fail;
+	}
+	addr = do_mmap_pgoff(file, addr, PAGE_SIZE, PROT_EXEC, MAP_PRIVATE, 0);
+	fput(file);
+
+	if (addr & ~PAGE_MASK) {

kernel/uprobes.c +4 -0

+	up_write(&mm->mmap_sem);
+	mmput(mm);
+	return 0;
+

+ * Called with mm->uproc->mutex locked.

+ * Returns the allocated area or NULL.
+ */

+/*
+ * xol_free_area - Free the area allocated for slots.

+ * @xol_area refers the unique per process uprobes_xol_area for
+ * this process.
+ *
+ */

+/*
+ * Find a slot
+ * - searching in existing vmas for a free slot.
+ * - If no free slot in existing vmas, return 0;

+ * Called when holding xol_area->lock

kernel/uprobes.c +30 -0

+		slot_addr = area->vaddr +
+				(slot_nr * UPROBES_XOL_SLOT_BYTES);
+		return slot_addr;
+	}
+
+	return 0;
+}
+
+/*
+ * xol_get_insn_slot - If user_bkpt  was not allocated a slot, then
+ * allocate a slot. If uprobes_insert_bkpt is already called, (i.e
+ * user_bkpt.vaddr != 0) then copy the instruction into the slot.
+ * @user_bkpt: probepoint information
+ * @xol_area refers the unique per process uprobes_xol_area for
+ * this process.
+ *
+ * Called with mm->uproc->mutex locked.
+ * Returns the allocated slot address or 0.
+ */
+static unsigned long xol_get_insn_slot(struct user_bkpt *user_bkpt,
+				struct uprobes_xol_area *xol_area)
+{
+	unsigned long flags, xol_vaddr = 0;
+	int len;
+
+	if (unlikely(!xol_area))
+		return 0;
+
+	if (user_bkpt->xol_vaddr)
+		return user_bkpt->xol_vaddr;

kernel/uprobes.c +28 -0

+	user_bkpt->xol_vaddr = xol_vaddr;
+	return user_bkpt->xol_vaddr;
+}
+
+/*
+ * xol_free_insn_slot - If slot was earlier allocated by
+ * @xol_get_insn_slot(), make the slot available for
+ * subsequent requests.
+ * @slot_addr: slot address as returned by
+ * @xol_get_insn_area().
+ * @xol_area refers the unique per process uprobes_xol_area for
+ * this process.
+ */
+static void xol_free_insn_slot(unsigned long slot_addr,
+				struct uprobes_xol_area *xol_area)
+{
+	unsigned long vma_end;
+	int found = 0;
+
+	if (unlikely(!slot_addr || IS_ERR_VALUE(slot_addr)))
+		return;
+
+	if (unlikely(!xol_area))
+		return;
+
+	vma_end = xol_area->vaddr + PAGE_SIZE;
+	if (xol_area->vaddr <= slot_addr && slot_addr < vma_end) {
+		int slot_nr;

kernel/uprobes.c +32 -0

+}
+
+/*
+ * xol_validate_vaddr - Verify if the specified address is in an
+ * executable vma, but not in an XOL vma.
+ *	- Return 0 if the specified virtual address is in an
+ *	  executable vma, but not in an XOL vma.
+ *	- Return 1 if the specified virtual address is in an
+ *	  XOL vma.
+ *	- Return -EINTR otherwise.(i.e non executable vma, or
+ *	  not a valid address
+ * @pid: the probed process
+ * @vaddr: virtual address of the instruction to be validated.
+ * @xol_area refers the unique per process uprobes_xol_area for
+ * this process.
+ */
+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)

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 477

Quoted 3 lines

kernel/uprobes.c +3 -0

+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;

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 483

Quoted 6 lines

kernel/uprobes.c +5 -0

+	/*
+	 * 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.

Anyway, you could install your own vm_ops and provide a close method to track this.

Quoted 3 lines

kernel/uprobes.c +3 -0

+	unsigned long vaddr;		/* Page(s) of instruction slots */
+};
+

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 532

Quoted 5 lines

kernel/uprobes.c +5 -0

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

Quoted 5 lines

+/*
+ * 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.

Quoted 2 lines

+ * Called with mm->uproc->mutex locked.

There's a nice way to not have to write that:

  lockdep_assert_held(&mm->uproc->mutex);

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

Quoted 1 line

+ * Called when holding xol_area->lock

  lockdep_assert_held(&area->lock);

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 602

Quoted 9 lines

+ */

kernel/uprobes.c +8 -0

+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?

Quoted 9 lines

kernel/uprobes.c +8 -0

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

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 642

Quoted 5 lines

kernel/uprobes.c +5 -0

+	/*
+	 * 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.

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

Quoted 6 lines

kernel/uprobes.c +6 -0

+		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);
+	}

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 654

Quoted 6 lines

kernel/uprobes.c +6 -0

+
+	/*
+	 * Update user_bkpt->xol_vaddr after giving a chance for the slot to
+	 * be initialized.
+	 */
+	mb();

Where is the matching barrier?

Quoted 5 lines

kernel/uprobes.c +4 -0

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

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 699

Quoted 8 lines

kernel/uprobes.c +7 -0

+		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?

Quoted 22 lines

kernel/uprobes.c +21 -0

+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;

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 741

Quoted 5 lines

kernel/uprobes.c +5 -0

+
+validate_end:
+	put_task_struct(tsk);
+	return result;
+}

This doesn't actually appear used in this patch,.. does it want to live elsewhere?

Quoted 3 lines

Quoted 1 line

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

Quoted 7 lines · Srikar Dronamraju

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

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.

If you want a compiler barrier, use barrier(), but here you seem to describe a multi-threaded situation, in which case the observer thread needs at least a rmb() in order for that mb() to mean anything other than the compiler barrier it implies.

Also, use smp_* barriers.

static unsigned long xol_get_insn_slot(struct user_bkpt *user_bkpt,
struct uprobes_xol_area *xol_area)

{
        unsigned long flags, xol_vaddr = 0;
        int len;

        if (unlikely(!xol_area))
                return 0;

        smp_rmb();
        if (user_bkpt->xol_vaddr)
                return user_bkpt->xol_vaddr;

        spin_lock_irqsave(&xol_area->lock, flags);
        xol_vaddr = xol_take_insn_slot(xol_area);
        spin_unlock_irqrestore(&xol_area->lock, flags);

        /*
         * Initialize the slot if user_bkpt->vaddr points to valid
         * instruction slot.
         */
        if (!xol_vaddr)
                return 0;

        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.
         */
        smp_mb();
        user_bkpt->xol_vaddr = xol_vaddr;
        return user_bkpt->xol_vaddr;
} 

Quoted 31 lines

static unsigned long xol_get_insn_slot(struct user_bkpt *user_bkpt,
struct uprobes_xol_area *xol_area)

{
        unsigned long flags, xol_vaddr = 0;
        int len;

        if (unlikely(!xol_area))
                return 0;

        smp_rmb();
        if (user_bkpt->xol_vaddr)
                return user_bkpt->xol_vaddr;

        spin_lock_irqsave(&xol_area->lock, flags);
        xol_vaddr = xol_take_insn_slot(xol_area);
        spin_unlock_irqrestore(&xol_area->lock, flags);

        /*
         * Initialize the slot if user_bkpt->vaddr points to valid
         * instruction slot.
         */
        if (!xol_vaddr)
                return 0;

        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);

Quoted 8 lines

        /*
         * Update user_bkpt->xol_vaddr after giving a chance for the slot to
         * be initialized.
         */
        smp_mb();
        user_bkpt->xol_vaddr = xol_vaddr;
        return user_bkpt->xol_vaddr;
} 

Racy like you won't believe..

Suppose multiple threads hitting the trap at the same time, every thread will end up failing the check and allocating a new slot for it, at the end the slowest thread will end up setting the value.

Quoted 8 lines

kernel/uprobes.c static int bad_arch_param(const char *param_name, int value) · line 532

Quoted 5 lines

kernel/uprobes.c +5 -0

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

Current slot allocation mechanism:
1. Allocate one dedicated slot per user breakpoint. Each slot is big enuf to accomodate the biggest instruction for that architecture. (16 bytes for x86).
2. We currently allocate only one page for slots. Hence the number of slots is limited to active breakpoint hits on that process.
3. Bitmap to track used slots.

Quoted 7 lines

Current slot allocation mechanism:
1. Allocate one dedicated slot per user breakpoint. Each slot is big enuf to accomodate the biggest instruction for that architecture. (16 bytes for x86).
2. We currently allocate only one page for slots. Hence the number of slots is limited to active breakpoint hits on that process.
3. Bitmap to track used slots.

An alternative method would be to have 1 slot per cpu, and manage the slot content using preemption notifiers. That gives you a fixed number of slots and an unlimited number of probe points.

If the preemption happens to be a migration you need to rewrite the userspace IP to point to the new slot -- if indeed the task was inside one when it got preempted -- but that all should be doable.

Quoted 12 lines

Quoted 7 lines

Current slot allocation mechanism:
1. Allocate one dedicated slot per user breakpoint. Each slot is big enuf to accomodate the biggest instruction for that architecture. (16 bytes for x86).
2. We currently allocate only one page for slots. Hence the number of slots is limited to active breakpoint hits on that process.
3. Bitmap to track used slots.

An alternative method would be to have 1 slot per cpu, and manage the slot content using preemption notifiers. That gives you a fixed number of slots and an unlimited number of probe points.

Quoted 4 lines

If the preemption happens to be a migration you need to rewrite the userspace IP to point to the new slot -- if indeed the task was inside one when it got preempted -- but that all should be doable.

Certainly doable but it has its share of drawbacks.
1. On every probe hit we have to copy the instruction into the slot, so there is a performance penalty.

2 This might complicate booster probe, because the jump instruction that follows the original instruction now actually have to coded every time.

3. Yes migration is an issue esp
- if a thread of the same process that hit a breakpoint is scheduled into the same cpu and that newly scheduled thread hits a breakpoint.
- Something similar can happen if a multithreaded process runs on a
uniprocessor machine.

4. I dont see a need for clearing slots after post processing, but if we need to clear we then are adding more penalties because not only are we clearing the slots but the post processing then cant happen in interrupt context.

5. I think we are covered on the cpu hotplug too, (i.e not sure if we have to make uprobes cpu hot plug aware.).

6. We would still be allocating a page for the slots. Unless we want to expand to more slots than available in one page, I dont see the disadvantages with the current approach.

Quoted 13 lines

Quoted 8 lines

An alternative method would be to have 1 slot per cpu, and manage the slot content using preemption notifiers. That gives you a fixed number of slots and an unlimited number of probe points.

If the preemption happens to be a migration you need to rewrite the userspace IP to point to the new slot -- if indeed the task was inside one when it got preempted -- but that all should be doable.

3. Yes migration is an issue esp
- if a thread of the same process that hit a breakpoint is scheduled into the same cpu and that newly scheduled thread hits a breakpoint.
- Something similar can happen if a multithreaded process runs on a
uniprocessor machine.

Quoted 2 lines

5. I think we are covered on the cpu hotplug too, (i.e not sure if we have to make uprobes cpu hot plug aware.).

Not if you use a slot per cpu and use preemption notifiers, the preemption notifiers will migrate the slots around.

Quoted 4 lines

6. We would still be allocating a page for the slots. Unless we want to expand to more slots than available in one page, I dont see the disadvantages with the current approach.

The current approach limits the number of probes to what fits in a page. The slot per cpu approach will have no such limit.

Quoted 3 lines

2 This might complicate booster probe, because the jump instruction that follows the original instruction now actually have to coded every time.

Why can't you keep the whole replacement sequence in-tact? Simply copy it out into the slot each time.

Lets say the thread while singlestepping the process gets pre-empted. Eventually the cpu might run some other thread of the same process before picking the first run thread. Or the first run thread could after migration due to load balancing or whatever end up running on a different thread? How do we handle these cases?

Quoted 2 lines

The current approach limits the number of probes to what fits in a page. The slot per cpu approach will have no such limit.

yes the limit on number of probes is a limitation. For now the implementation would be straight and easy. We could either rework on the algorithm or add more pages depending on how often uprobes gets used.

Provides x86 specific functions for instruction analysis and instruction validation and x86 specific pre-processing and post-processing of singlestep especially for RIP relative instructions. Uses "x86: instruction decoder API" for validation and analysis of user space instructions. This analysis is used at the time of post-processing of breakpoint hit to do the necessary fix-ups. There is support for breakpointing RIP relative instructions. However there are still few instructions that cannot be singlestepped.

+typedef u8 user_bkpt_opcode_t;

arch/x86/kernel/uprobes.c new file +17 -0

+
+#define UPROBES_FIX_RIP_AX	0x8000
+#define UPROBES_FIX_RIP_CX	0x4000
+
+/* Adaptations for mhiramat x86 decoder v14. */
+#define OPCODE1(insn) ((insn)->opcode.bytes[0])
+#define OPCODE2(insn) ((insn)->opcode.bytes[1])
+#define OPCODE3(insn) ((insn)->opcode.bytes[2])
+#define MODRM_REG(insn) X86_MODRM_REG(insn->modrm.value)
+
+/*
+ * @reg: reflects the saved state of the task
+ * @vaddr: the virtual address to jump to.
+ * Return 0 on success or a -ve number on error.
+ */
+void set_ip(struct pt_regs *regs, unsigned long vaddr)
+{

arch/x86/kernel/uprobes.c new file +10 -0

+
+static void report_bad_prefix(void)
+{
+	printk(KERN_ERR "uprobes does not currently support probing "
+		"instructions with any of the following prefixes: "
+		"cs:, ds:, es:, ss:, lock:\n");
+}
+
+static void report_bad_1byte_opcode(int mode, user_bkpt_opcode_t op)
+{

arch/x86/kernel/uprobes.c new file +8 -0

+
+/**
+ * analyze_insn - instruction analysis including validity and fixups.
+ * @tsk: the probed task.
+ * @user_bkpt: the probepoint information.
+ * Return 0 on success or a -ve number on error.
+ */
+int analyze_insn(struct task_struct *tsk, struct user_bkpt *user_bkpt)

+ */

arch/x86/kernel/uprobes.c new file +3 -0

+static int adjust_ret_addr(struct task_struct *tsk, unsigned long sp,
+							long correction)
+{

One general comment here: since with uprobes the instruction decoder becomes security critical did you do any fuzz tests on it (e.g. like using it on crashme or on code that has been corrupted with a few bitflips) ?

Quoted 2 lines

+typedef u8 user_bkpt_opcode_t;

Maybe it's me, but I would prefer breakpoint instead of bkpt

arch/x86/kernel/uprobes.c line 35

Quoted 5 lines

arch/x86/kernel/uprobes.c new file +5 -0

+#ifdef CONFIG_X86_32
+#define is_32bit_app(tsk) 1
+#else
+#define is_32bit_app(tsk) (test_tsk_thread_flag(tsk, TIF_IA32))
+#endif

This probably should be elsewhere.

Quoted 18 lines

arch/x86/kernel/uprobes.c new file +17 -0

+
+#define UPROBES_FIX_RIP_AX	0x8000
+#define UPROBES_FIX_RIP_CX	0x4000
+
+/* Adaptations for mhiramat x86 decoder v14. */
+#define OPCODE1(insn) ((insn)->opcode.bytes[0])
+#define OPCODE2(insn) ((insn)->opcode.bytes[1])
+#define OPCODE3(insn) ((insn)->opcode.bytes[2])
+#define MODRM_REG(insn) X86_MODRM_REG(insn->modrm.value)
+
+/*
+ * @reg: reflects the saved state of the task
+ * @vaddr: the virtual address to jump to.
+ * Return 0 on success or a -ve number on error.
+ */
+void set_ip(struct pt_regs *regs, unsigned long vaddr)
+{

arch/x86/kernel/uprobes.c line 62

Quoted 10 lines

arch/x86/kernel/uprobes.c new file +10 -0

+	regs->ip = vaddr;
+}
+
+#ifdef CONFIG_X86_64
+static bool is_riprel_insn(struct user_bkpt *user_bkpt)
+{
+	return ((user_bkpt->fixups &
+			(UPROBES_FIX_RIP_AX | UPROBES_FIX_RIP_CX)) != 0);
+}
+

Shouldn't all this stuff be in the instruction decoder?

It seems weird to have the knowledge spread over multiple files.

Quoted 17 lines

arch/x86/kernel/uprobes.c new file +16 -0

+
+static void report_bad_prefix(void)
+{
+	printk(KERN_ERR "uprobes does not currently support probing "
+		"instructions with any of the following prefixes: "
+		"cs:, ds:, es:, ss:, lock:\n");
+}
+
+static void report_bad_1byte_opcode(int mode, user_bkpt_opcode_t op)
+{
+	printk(KERN_ERR "In %d-bit apps, "
+		"uprobes does not currently support probing "
+		"instructions whose first byte is 0x%2.2x\n", mode, op);
+}
+
+static void report_bad_2byte_opcode(user_bkpt_opcode_t op)

arch/x86/kernel/uprobes.c line 227

Quoted 4 lines

arch/x86/kernel/uprobes.c new file +4 -0

+{
+	printk(KERN_ERR "uprobes does not currently support probing "
+		"instructions with the 2-byte opcode 0x0f 0x%2.2x\n", op);
+}

These functions that just do a single printk seem weird. I would do that in the caller. Also the message could be shortened I guess and should just dump the bytes.

Quoted 16 lines

arch/x86/kernel/uprobes.c new file +16 -0

+
+/**
+ * analyze_insn - instruction analysis including validity and fixups.
+ * @tsk: the probed task.
+ * @user_bkpt: the probepoint information.
+ * Return 0 on success or a -ve number on error.
+ */
+int analyze_insn(struct task_struct *tsk, struct user_bkpt *user_bkpt)
+{
+	int ret;
+	struct insn insn;
+
+	user_bkpt->fixups = 0;
+#ifdef CONFIG_X86_64
+	user_bkpt->arch_info.rip_target_address = 0x0;
+#endif

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	if (is_32bit_app(tsk))

This check is not fully correct because it's valid to have 32bit code in 64bit programs and vice versa. The only good way to check that is to look at the code segment at runtime though (and it gets complicated if you want to handle LDTs, but that could be optional). May be difficult to do though.

Also the compat bit is not necessarily set if no system call is executing. You would rather need to check the exec_domain.

Quoted 8 lines

+ */

arch/x86/kernel/uprobes.c new file +7 -0

+static int adjust_ret_addr(struct task_struct *tsk, unsigned long sp,
+							long correction)
+{
+	int rasize, ncopied;
+	long ra = 0;
+
+	if (is_32bit_app(tsk))

arch/x86/kernel/uprobes.c line 485

Quoted 6 lines

arch/x86/kernel/uprobes.c new file +6 -0

+		rasize = 4;
+	else
+		rasize = 8;
+	ncopied = uprobes_read_vm(tsk, (void __user *) sp, &ra, rasize);
+	if (unlikely(ncopied != rasize))
+		goto fail;

goto is automatically unlikely and unlikely is deprecated anyways.

Quoted 1 line

Maybe it's me, but I would prefer breakpoint instead of bkpt

Even Peter wasnt comfortable with user_bkpt.

Quoted 5 lines

One general comment here: since with uprobes the instruction decoder becomes security critical did you do any fuzz tests on it (e.g. like using it on crashme or on code that has been corrupted with a few bitflips) ?

I havent tried any fuzz tests with the instruction decoder. But I am not sure if Masami has tried that out some of these.
One question: Do you want to test uprobes with crashme or test instruction decoder with crashme.

Quoted 7 lines

arch/x86/kernel/uprobes.c line 35

Quoted 5 lines

arch/x86/kernel/uprobes.c new file +5 -0

+#ifdef CONFIG_X86_32
+#define is_32bit_app(tsk) 1
+#else
+#define is_32bit_app(tsk) (test_tsk_thread_flag(tsk, TIF_IA32))
+#endif

This probably should be elsewhere.

Would this fit in x86 Instruction decoder?

Okay, I can move the printk to the caller, I will try to shorten the message, Would something like "uprobes: no support for 2-byte opcode 0x0f 0x%2" look fine?

Quoted 5 lines

This check is not fully correct because it's valid to have 32bit code in 64bit programs and vice versa. The only good way to check that is to look at the code segment at runtime though (and it gets complicated if you want to handle LDTs, but that could be optional). May be difficult to do though.

validate_insn_32bit is able to identify all valid instructions in a 32 bit app and validate_insn_64bits is a superset of validate_insn_32bits; i.e it considers valid 32 bit codes as valid too.

Did you get a chance to look at validate_insn_32bit/validate_insn_64bits? If you feel that validate_insn_32bit/validate_insn_64bits? are unable to detect valid codes, then I will certainly rework.

Quoted 2 lines

Also the compat bit is not necessarily set if no system call is executing. You would rather need to check the exec_domain.

Okay, I shall check and revert on this.

Quoted 4 lines

I havent tried any fuzz tests with the instruction decoder. But I am not sure if Masami has tried that out some of these.
One question: Do you want to test uprobes with crashme or test instruction decoder with crashme.

Ideally both, but as a minimum the part that is exposed to user space, that is uprobes.

Quoted 6 lines

validate_insn_32bit is able to identify all valid instructions in a 32 bit app and validate_insn_64bits is a superset of validate_insn_32bits; i.e it considers valid 32 bit codes as valid too.

How can this be? e.g. 32bit has 1 byte INC/DEC but on 64bit these are REX prefixes and can be in front of nearly anything. So a super set cannot be correct. It has to be either / or.

Quoted 5 lines

Did you get a chance to look at validate_insn_32bit/validate_insn_64bits? If you feel that validate_insn_32bit/validate_insn_64bits? are unable to detect valid codes, then I will certainly rework.

I don't think you can do a 100% solution because for 100% you would need to know the code segment the CPU is going to use later, and that's not possible in advance.

A heuristic is reasonable (and leave out applications that generate 64bit code from 32bit executables or vice versa) but you need to test the right personality bits for that.

Quoted 4 lines

Quoted 2 lines

Also the compat bit is not necessarily set if no system call is executing. You would rather need to check the exec_domain.

Okay, I shall check and revert on this.

Hmm actually I double checked and this is a separate bit. So scratch that, TIF_32BIT is ok to test.

[adding Masami and Jim to the copy list]

Quoted 7 lines

Quoted 4 lines

I havent tried any fuzz tests with the instruction decoder. But I am not sure if Masami has tried that out some of these. One question: Do you want to test uprobes with crashme or test instruction decoder with crashme.

Ideally both, but as a minimum the part that is exposed to user space, that is uprobes.

Okay, I will test uprobes with crashme.

Quoted 6 lines

Quoted 5 lines

Did you get a chance to look at validate_insn_32bit/validate_insn_64bits? If you feel that validate_insn_32bit/validate_insn_64bits? are unable to detect valid codes, then I will certainly rework.

Quoted 4 lines

I don't think you can do a 100% solution because for 100% you would need to know the code segment the CPU is going to use later, and that's not possible in advance.

I think you are referring to RIP related instructions, this how we handle them.

[adding Masami and Jim to the copy list]

Quoted 4 lines

Quoted 4 lines

I havent tried any fuzz tests with the instruction decoder. But I am not sure if Masami has tried that out some of these.
One question: Do you want to test uprobes with crashme or test instruction decoder with crashme.

Quoted 2 lines

Ideally both, but as a minimum the part that is exposed to user space, that is uprobes.

Okay, I will test uprobes with crashme.

Quoted 11 lines

Quoted 6 lines

validate_insn_32bit is able to identify all valid instructions in a 32 bit app and validate_insn_64bits is a superset of validate_insn_32bits; i.e it considers valid 32 bit codes as valid too.

How can this be? e.g. 32bit has 1 byte INC/DEC but on 64bit these are REX prefixes and can be in front of nearly anything. So a super set cannot be correct. It has to be either / or.

You are right, the validate_insn_32bits refers to good_insns_32 and validate_insn_64bits refers to good_insns_64 to decode 1 byte instructions. Some instructions like 0x06 and 0x0e seem to be valid in good_insns_32 but not in good_insns_64.

Quoted 9 lines

Quoted 4 lines

Did you get a chance to look at validate_insn_32bit/validate_insn_64bits? If you feel that validate_insn_32bit/validate_insn_64bits? are unable to detect valid codes, then I will certainly rework.

I don't think you can do a 100% solution because for 100% you would need to know the code segment the CPU is going to use later, and that's not possible in advance.

I think you are referring to RIP related instructions, this how we handle them.
Please correct us if we are wrong, but here is what we do
- While analyzing the instruction, take into account which register acts as the code segment register.

- When interrupted (but before singlestep), copy the contents of the register which we think acts as code segment register in our above analysis into per-task scratch variable.

- After singlestepping we retrieve the saved per-task scratch variable into the corresponding register.

Quoted 5 lines

A heuristic is reasonable (and leave out applications that generate 64bit code from 32bit executables or vice versa) but you need to test the right personality bits for that.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Also the compat bit is not necessarily set if no system call is executing. You would rather need to check the exec_domain.

Okay, I shall check and revert on this.

Hmm actually I double checked and this is a separate bit. So scratch that, TIF_32BIT is ok to test.

Okay, Thanks for confirming this.

TODO:
1. Allow multiple probes at a probepoint.
2. Booster probes.
3. Allow probes to be inherited across fork.

4. probing function returns.

Signed-off-by Srikar Dronamraju <[email protected]>

Signed-off-by Jim Keniston <[email protected]>

---

 arch/Kconfig             |    1 
 fs/exec.c                |    4 
 include/linux/mm_types.h |    4 
 include/linux/sched.h    |    3 
 include/linux/uprobes.h  |  143 ++++++++++
 kernel/fork.c            |   21 +
 kernel/uprobes.c         |  653 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 826 insertions(+), 3 deletions(-)

arch/Kconfig +0 -0

index 87bd26b..c8c8e3f 100644

fs/exec.c +0 -0

 }
 EXPORT_SYMBOL(setup_new_exec);
 

include/linux/mm_types.h +1 -0

index cb57d65..8f79e51 100644
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
+#include <linux/uprobes.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 

+/*
+ * uprobe_process -- not a user-visible struct.

+ * A uprobe_process represents a probed process. A process can have
+ * multiple probepoints (each represented by a uprobe_probept) and
+ * one or more threads (each represented by a uprobe_task).
+ *
+ * All processes/threads that share a mm share the same uprobe_process.
+ */
+struct uprobe_process {

include/linux/sched.h +4 -0

+	/*
+	 * mutex locked for any change to the uprobe_process's
+	 * graph (including uprobe_probept, taking a slot in xol_area) --
+	 * e.g., due to probe [un]registration or special events like exit.

include/linux/sched.h +6 -0

+	/* breakpoint/XOL details */
+	struct user_bkpt user_bkpt;
+
+	/*
+	 * ppt goes in the uprobe_process->uprobe_table when registered --
+	 * even before the breakpoint has been inserted.

include/linux/sched.h +8 -0

+enum uprobe_task_state {
+	UTASK_RUNNING,
+	UTASK_BP_HIT,
+	UTASK_SSTEP
+};
+
+/*
+ * uprobe_utask -- not a user-visible struct.

--git a/kernel/fork.c b/kernel/fork.c
index b7e9d60..d78cf5d 100644

kernel/fork.c +15 -0

@@ -187,6 +187,11 @@ void __put_task_struct(struct task_struct *tsk)
 	delayacct_tsk_free(tsk);
 	put_signal_struct(tsk->signal);
 
+#ifdef CONFIG_UPROBES
+	if (unlikely(tsk->utask))
+		uprobe_free_utask(tsk);
+#endif
+
 	if (!profile_handoff_task(tsk))
 		free_task(tsk);
 }

@@ -525,6 +530,10 @@ void __mmdrop(struct mm_struct *mm)
 	mm_free_pgd(mm);
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
+#ifdef CONFIG_UPROBES
+	if (unlikely(mm->uproc))
+		uprobe_put_uprocess(mm);
+#endif
 	free_mm(mm);
 }
 EXPORT_SYMBOL_GPL(__mmdrop);

@@ -683,6 +692,9 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 	if (mm->binfmt && !try_module_get(mm->binfmt->module))
 		goto free_pt;
 
+#ifdef CONFIG_UPROBES
+	mm->uproc = NULL;
+#endif
 	return mm;
 
 free_pt:

@@ -1190,6 +1202,9 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	INIT_LIST_HEAD(&p->pi_state_list);
 	p->pi_state_cache = NULL;
 #endif
+#ifdef CONFIG_UPROBES
+	p->utask = NULL;
+#endif
 	/*
 	 * sigaltstack should be cleared when sharing the same VM
 	 */

@@ -1288,6 +1303,12 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	proc_fork_connector(p);
 	cgroup_post_fork(p);

kernel/uprobes.c +6 -2

index 230adf3..38c7abb 100644
@@ -21,6 +21,7 @@
  *	Jim Keniston
  */
 #include <linux/kernel.h>
+#include <linux/types.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/sched.h>

@@ -36,6 +37,9 @@
 #include <linux/file.h>
 #include <linux/pid.h>
 #include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/kdebug.h>
+
 
 struct user_bkpt_arch_info *arch = &user_bkpt_arch_info;
 

@@ -345,7 +349,7 @@ static int __insert_bkpt(struct task_struct *tsk,
  *	@__insert_bkpt(). @user_bkpt->xol_vaddr must be the
  *	address of an XOL instruction slot that is allocated to this
  *	probepoint at least until after the completion of
- *	@uprobes_post_sstep(), and populated with the contents of
+ *	@post_sstep(), and populated with the contents of
  *	@user_bkpt->insn.
  *	@tskinfo: points to a @user_bkpt_task_arch_info object for @tsk.
  *	@regs: reflects the saved user state of @tsk.  pre_sstep()

@@ -356,7 +360,7 @@ static int __insert_bkpt(struct task_struct *tsk,
  *
  * The client must ensure that the contents of @user_bkpt are not
  * changed during the single-step operation -- i.e., between when
- * @uprobes_pre_sstep() is called and when @uprobes_post_sstep() returns.
+ * @pre_sstep() is called and when @post_sstep() returns.
  */
 static int pre_sstep(struct task_struct *tsk, struct user_bkpt *user_bkpt,
 		struct user_bkpt_task_arch_info *tskinfo, struct pt_regs *regs)

kernel/uprobes.c +0 -0

@@ -741,6 +745,642 @@ validate_end:
 }
 /* end of slot allocation for XOL */
 

kernel/uprobes.c +6 -0

+ */
+void uprobe_handle_fork(struct task_struct *child)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;
+	int ret;

kernel/uprobes.c +39 -0

+	mutex_lock(&uproc->mutex);
+	list_for_each_entry(ppt, &uproc->uprobe_list, ut_node) {
+		ret = __remove_bkpt(child, &ppt->user_bkpt);
+		if (ret && ret != -EINVAL) {
+			/* Ratelimit this? */
+			printk(KERN_ERR "Pid %d forked %d; failed to"
+				" remove probepoint at %#lx in child\n",
+				current->pid, child->pid,
+				ppt->user_bkpt.vaddr);
+		}
+	}
+	mutex_unlock(&uproc->mutex);
+}
+
+/*
+ * uprobe_notify_resume gets called in task context just before returning
+ * to userspace.
+ *
+ *  If its the first time the probepoint is hit, slot gets allocated here.
+ *  If its the first time the thread hit a breakpoint, utask gets
+ *  allocated here.
+ */
+void uprobe_notify_resume(struct pt_regs *regs)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;
+	struct uprobe_task *utask;
+	struct uprobe *u;
+	unsigned long probept;
+
+	utask = current->utask;
+	uproc = current->mm->uproc;
+	if (unlikely(!utask)) {
+		utask = add_utask(uproc);
+
+		/* Failed to allocate utask for the current task. */
+		BUG_ON(!utask);
+		probept = uprobes_get_bkpt_addr(regs);
+		ppt = find_probept(uproc, probept);

kernel/uprobes.c +26 -0

+	if (utask->state == UTASK_BP_HIT) {
+		utask->state = UTASK_SSTEP;
+		u = ppt->uprobe;
+		if (u && u->handler)
+			u->handler(u, regs);
+
+		if (!pre_ssout(ppt, regs))
+			arch_uprobe_enable_sstep(regs);
+	} else if (utask->state == UTASK_SSTEP) {
+		if (sstep_complete(regs, ppt)) {
+			put_probept(ppt);
+			utask->active_ppt = NULL;
+			utask->state = UTASK_RUNNING;
+			arch_uprobe_disable_sstep(regs);
+		}
+	}
+}
+
+/*
+ * uprobe_bkpt_notifier gets called from interrupt context
+ * it gets a reference to the ppt and sets TIF_UPROBE flag,
+ */
+int uprobe_bkpt_notifier(struct pt_regs *regs)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;

kernel/uprobes.c +45 -0

+	return 1;
+}
+
+/*
+ * uprobe_post_notifier gets called in interrupt context.
+ * It completes the single step operation.
+ */
+int uprobe_post_notifier(struct pt_regs *regs)
+{
+	struct uprobe_probept *ppt;
+	struct uprobe_task *utask;
+
+	if (!current->mm || !current->mm->uproc || !current->utask)
+		/* task is currently not uprobed */
+		return 0;
+
+	utask = current->utask;
+
+	ppt = utask->active_ppt;
+	if (!ppt)
+		return 0;
+
+	if (uprobes_resume_can_sleep(&ppt->user_bkpt)) {
+#ifdef CONFIG_ARCH_SUPPORTS_UPROBES
+		set_thread_flag(TIF_UPROBE);
+#endif
+		return 1;
+	}
+	if (sstep_complete(regs, ppt)) {
+		put_probept(ppt);
+		arch_uprobe_disable_sstep(regs);
+		utask->active_ppt = NULL;
+		utask->state = UTASK_RUNNING;
+		return 1;
+	}
+	return 0;
+}
+
 static int __init init_uprobes(void)
 {
 	int result = 0;

@@ -752,7 +1392,14 @@ static int __init init_uprobes(void)
 	if (arch->ip_advancement_by_bkpt_insn > arch->max_insn_bytes)
 		result = bad_arch_param("ip_advancement_by_bkpt_insn",
 				arch->ip_advancement_by_bkpt_insn);
+
+	register_die_notifier(&uprobes_exception_nb);
 	return result;
 }
 
+static void __exit exit_uprobes(void)
+{
+}
+
 module_init(init_uprobes);
+module_exit(exit_uprobes);

Quoted 17 lines

kernel/uprobes.c +17 -0

+void uprobe_notify_resume(struct pt_regs *regs)
+{
+     struct uprobe_process *uproc;
+     struct uprobe_probept *ppt;
+     struct uprobe_task *utask;
+     struct uprobe *u;
+     unsigned long probept;
+
+     utask = current->utask;
+     uproc = current->mm->uproc;
+     if (unlikely(!utask)) {
+             utask = add_utask(uproc);
+
+             /* Failed to allocate utask for the current task. */
+             BUG_ON(!utask);
+             probept = uprobes_get_bkpt_addr(regs);
+             ppt = find_probept(uproc, probept);

kernel/uprobes.c validate_end: · line 1297

Quoted 10 lines

kernel/uprobes.c +10 -0

+
+             /*
+              * The probept was refcounted in uprobe_bkpt_notifier;
+              * Hence it would be mysterious to miss ppt now
+              */
+             WARN_ON(!ppt);
+             utask->active_ppt = ppt;
+             utask->state = UTASK_BP_HIT;
+     } else
+             ppt = utask->active_ppt;

You can replace this with:

addr = instruction_pointer(task_pt_regs(current)) -
ip_advancement_by_brkpt_insn;

and then proceed from there like described below to obtain the struct uprobe.

You can infer the SS/HIT state by checking if the user-addr is in the XOL area or not.

Quoted 26 lines

kernel/uprobes.c +26 -0

+     if (utask->state == UTASK_BP_HIT) {
+             utask->state = UTASK_SSTEP;
+             u = ppt->uprobe;
+             if (u && u->handler)
+                     u->handler(u, regs);
+
+             if (!pre_ssout(ppt, regs))
+                     arch_uprobe_enable_sstep(regs);
+     } else if (utask->state == UTASK_SSTEP) {
+             if (sstep_complete(regs, ppt)) {
+                     put_probept(ppt);
+                     utask->active_ppt = NULL;
+                     utask->state = UTASK_RUNNING;
+                     arch_uprobe_disable_sstep(regs);
+             }
+     }
+}
+
+/*
+ * uprobe_bkpt_notifier gets called from interrupt context
+ * it gets a reference to the ppt and sets TIF_UPROBE flag,
+ */
+int uprobe_bkpt_notifier(struct pt_regs *regs)
+{
+     struct uprobe_process *uproc;
+     struct uprobe_probept *ppt;

kernel/uprobes.c validate_end: · line 1334

Quoted 10 lines

kernel/uprobes.c +10 -0

+     struct uprobe_task *utask;
+     unsigned long probept;
+
+     if (!current->mm || !current->mm->uproc)
+             /* task is currently not uprobed */
+             return 0;
+
+     uproc = current->mm->uproc;
+     utask = current->utask;
+     probept = uprobes_get_bkpt_addr(regs);

Its an address, not a struct uprobe_probept * so call it addr if anything.

kernel/uprobes.c validate_end: · line 1342

Quoted 8 lines

kernel/uprobes.c +8 -0

+     ppt = find_probept(uproc, probept);
+     if (!ppt)
+             return 0;
+     get_probept(ppt);
+     if (utask) {
+             utask->active_ppt = ppt;
+             utask->state = UTASK_BP_HIT;
+     }

Right, so all this should die..

Obtain the address, lookup the vma, get the file, compute the offset inside that file -> {inode,offset}

Do the lookup in the global tree to obtain the uprobe, and possibly execute the probe handler right here in interrupt context, if it fails with -EFAULT, continue with the below:

kernel/uprobes.c validate_end: · line 1345

Quoted 3 lines

kernel/uprobes.c +3 -0

+#ifdef CONFIG_ARCH_SUPPORTS_UPROBES
+     set_thread_flag(TIF_UPROBE);
+#endif

How can we ever get here if the architecture doesn't support uprobes?

Quoted 2 lines

(quoted diff) +2 -0

+     return 1;
+} 

Quoted 4 lines · Srikar Dronamraju

TODO:
1. Allow multiple probes at a probepoint.
2. Booster probes.
3. Allow probes to be inherited across fork.

I wouldn't worry about that, focus on inode attached probes and you get fork for free.

Quoted 6 lines

arch/Kconfig +0 -0

index 87bd26b..c8c8e3f 100644
@@ -49,6 +49,7 @@ config OPTPROBES
 

arch/Kconfig config OPTPROBES · line 55

Quoted 6 lines

arch/Kconfig +1 -0

 config UPROBES
 	bool "User-space probes (EXPERIMENTAL)"
+	default n
 	depends on ARCH_SUPPORTS_UPROBES
 	depends on MMU
 	help

Seems like a weird place for this hunk, does this want to live elsewhere?

arch/Kconfig

Quoted 4 lines

arch/Kconfig +4 -0

+#ifdef CONFIG_UPROBES
+	if (unlikely(current->utask))
+		uprobe_free_utask(current);
+#endif

A nicer way is to provide flush_uprobes() unconditionally.

arch/Kconfig

Quoted 5 lines

arch/Kconfig +3 -0

+#ifdef CONFIG_UPROBES
+	struct uprobe_process *uproc;	/* per mm uprobes info */
+#endif
 };

I find the _process postfix a bit weird in this context, how about something like:

struct mm_uprobes *mm_uprobes;

instead?

arch/Kconfig struct task_struct { · line 1502

Quoted 9 lines

arch/Kconfig +3 -0

@@ -1496,6 +1496,9 @@ struct task_struct {
 		unsigned long memsw_bytes; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+#ifdef CONFIG_UPROBES
+	struct uprobe_task *utask;
+#endif
 };

Make that:

	struct uprobe_task_state uprobe_state;

And provide an empty struct for !CONFIG_UPROBE, see below.

arch/Kconfig struct task_struct {

Quoted 8 lines

arch/Kconfig +7 -0

+/* This is what the user supplies us. */
+struct uprobe {
+	/*
+	 * The pid of the probed process.  Currently, this can be the
+	 * thread ID (task->pid) of any active thread in the process.
+	 */
+	pid_t pid;

Would fail for pid namespaces I guess...

Quoted 7 lines

arch/Kconfig +6 -0

+	/* Location of the probepoint */
+	unsigned long vaddr;
+
+	/* Handler to run when the probepoint is hit */
+	void (*handler)(struct uprobe*, struct pt_regs*);
+};

Quoted 2 lines

+/*
+ * uprobe_process -- not a user-visible struct.

Seems like a weird comment for kernel code..

arch/Kconfig

Quoted 3 lines

arch/Kconfig +2 -0

+	/* lock held while traversing/modifying uprobe_list and n_ppts */
+	spinlock_t pptlist_lock;	/* protects uprobe_list */

Its customary to write it like:

	spinlock_t	 list_lock; /* protects uprobe_list, nr_uprobes */
	struct list_head uprobe_list;
	int		 nr_uprobes;

arch/Kconfig

Quoted 9 lines

arch/Kconfig +9 -0

+
+	/* number of probept allocated for this process */
+	int n_ppts;
+
+	/*
+	 * Manages slots for instruction-copies to be single-stepped
+	 * out of line.
+	 */
+	void *xol_area;

Why void * and not simply:

	struct uprobes_xol_area xol_area;

That struct is small enough and you only get one per mm and saves you an allocation/pointer-chase.

arch/Kconfig

Quoted 8 lines

arch/Kconfig +8 -0

+};
+
+/*
+ * uprobe_probept -- not a user-visible struct.
+ * A uprobe_probept represents a probepoint.
+ * Guarded by uproc->lock.
+ */
+struct uprobe_probept {

you really got a way with names, uprobe_point would be so much better.

Quoted 7 lines

arch/Kconfig +6 -0

+	/* breakpoint/XOL details */
+	struct user_bkpt user_bkpt;
+
+	/*
+	 * ppt goes in the uprobe_process->uprobe_table when registered --
+	 * even before the breakpoint has been inserted.

arch/Kconfig

Quoted 10 lines

arch/Kconfig +10 -0

+	 */
+	struct list_head ut_node;
+
+	atomic_t refcount;
+
+	/* The parent uprobe_process */
+	struct uprobe_process *uproc;
+
+	struct uprobe *uprobe;
+};

So this thing is a link between the process and the probe, I'm not quite sure what you need the refcount for, it seems to me you can only have on of these per process/probe combination.

If you had used inode/offset based probes they would have been unique in the system and you could have had an {inode,offset} indexed global tree (or possibly a tree per inode, but that would mean adding to the inode structure, which I think is best avoided).

That would also reduce the mm state to purely the xol area, no need to manage this process to probe map.

Quoted 15 lines

arch/Kconfig +15 -0

+enum uprobe_task_state {
+	UTASK_RUNNING,
+	UTASK_BP_HIT,
+	UTASK_SSTEP
+};
+
+/*
+ * uprobe_utask -- not a user-visible struct.
+ * Corresponds to a thread in a probed process.
+ * Guarded by uproc->mutex.
+ */
+struct uprobe_task {
+	struct user_bkpt_task_arch_info arch_info;
+
+	enum uprobe_task_state state;

arch/Kconfig

Quoted 3 lines

arch/Kconfig +3 -0

+
+	struct uprobe_probept *active_ppt;
+};

I would be thinking you can obtain the active probe point from the address the task is stuck at and the state seems fairly redundant. Which leaves you with the arch state, which afaict is exactly as large as the pointer currently in the task struct.

Quoted 9 lines

arch/Kconfig +6 -0

 	perf_event_fork(p);
+#ifdef CONFIG_UPROBES
+	if ((current->mm) && !(clone_flags & CLONE_VM)) {
+		if (unlikely(current->mm->uproc))
+			uprobe_handle_fork(p);
+	}
+#endif
 	return p;

Quoted 1 line

bad_fork_free_pid:

All that can be replaced by unconditional functions, simply stub them out for !CONFIG_UPROBE.

Quoted 8 lines

/*
* xol_get_insn_slot - If user_bkpt was not allocated a slot, then
- * allocate a slot. If uprobes_insert_bkpt is already called, (i.e
+ * allocate a slot. If insert_bkpt is already called, (i.e
* user_bkpt.vaddr != 0) then copy the instruction into the slot.
* @user_bkpt: probepoint information
* @xol_area refers the unique per process uprobes_xol_area for

Wandering hunks, these seem to want to get folded back to wherever the original text comes from..

Quoted 4 lines

arch/Kconfig +0 -0

@@ -741,6 +745,642 @@ validate_end:
 }
 /* end of slot allocation for XOL */
 

arch/Kconfig validate_end: · line 757

Quoted 10 lines

arch/Kconfig +10 -0

+
+struct notifier_block uprobes_exception_nb = {
+	.notifier_call = uprobes_exception_notify,
+	.priority = 0x7ffffff0,
+};
+
+typedef void (*uprobe_handler_t)(struct uprobe*, struct pt_regs*);
+
+/* Guards lookup, creation, and deletion of uproc. */
+static DEFINE_MUTEX(uprobe_mutex);

uproc is per mm, why a global mutex?

arch/Kconfig validate_end: · line 767

Quoted 12 lines

arch/Kconfig +10 -0

+/*
+ * Fork callback: The current task has spawned a process.
+ * NOTE: For now, we don't pass on uprobes from the parent to the
+ * child. We now do the necessary clearing of breakpoints in the
+ * child's address space.
+ * This function handles the case where vm is not shared between
+ * the parent and the child.
+ *
+ * TODO:
+ *	- Provide option for child to inherit uprobes.

Don't bother with that...

Quoted 12 lines

arch/Kconfig +11 -0

+ */
+void uprobe_handle_fork(struct task_struct *child)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;
+	int ret;
+
+	uproc = current->mm->uproc;
+
+	/*
+	 * New process spawned by parent but not sharing the same mm.

arch/Kconfig validate_end: · line 783

Quoted 5 lines

arch/Kconfig +5 -0

+	 * Remove the probepoints in the child's text.
+	 *
+	 * We also hold the uproc->mutex for the parent - so no
+	 * new uprobes will be registered 'til we return.
+	 */

The grand thing about not having any of this process state is that you dont need to clean it up either..

Quoted 7 lines

arch/Kconfig +7 -0

+	if (unlikely(!utask)) {
+		utask = add_utask(uproc);
+
+		/* Failed to allocate utask for the current task. */
+		BUG_ON(!utask);
+		probept = uprobes_get_bkpt_addr(regs);
+		ppt = find_probept(uproc, probept);

arch/Kconfig validate_end: · line 800

Quoted 10 lines

arch/Kconfig +10 -0

+
+		/*
+		 * The probept was refcounted in uprobe_bkpt_notifier;
+		 * Hence it would be mysterious to miss ppt now
+		 */
+		WARN_ON(!ppt);
+		utask->active_ppt = ppt;
+		utask->state = UTASK_BP_HIT;
+	} else
+		ppt = utask->active_ppt;

You can replace this with:

addr = instruction_pointer(task_pt_regs(current)) -
ip_advancement_by_brkpt_insn;

and then proceed from there like described below to obtain the struct uprobe.

You can infer the SS/HIT state by checking if the user-addr is in the XOL area or not.

Quoted 31 lines

arch/Kconfig +31 -0

+	if (utask->state == UTASK_BP_HIT) {
+		utask->state = UTASK_SSTEP;
+		u = ppt->uprobe;
+		if (u && u->handler)
+			u->handler(u, regs);
+
+		if (!pre_ssout(ppt, regs))
+			arch_uprobe_enable_sstep(regs);
+	} else if (utask->state == UTASK_SSTEP) {
+		if (sstep_complete(regs, ppt)) {
+			put_probept(ppt);
+			utask->active_ppt = NULL;
+			utask->state = UTASK_RUNNING;
+			arch_uprobe_disable_sstep(regs);
+		}
+	}
+}
+
+/*
+ * uprobe_bkpt_notifier gets called from interrupt context
+ * it gets a reference to the ppt and sets TIF_UPROBE flag,
+ */
+int uprobe_bkpt_notifier(struct pt_regs *regs)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;
+	struct uprobe_task *utask;
+	unsigned long probept;
+
+	if (!current->mm || !current->mm->uproc)
+		/* task is currently not uprobed */

arch/Kconfig validate_end: · line 836

Quoted 5 lines

arch/Kconfig +5 -0

+		return 0;
+
+	uproc = current->mm->uproc;
+	utask = current->utask;
+	probept = uprobes_get_bkpt_addr(regs);

Its an address, not a struct uprobe_probept * so call it addr if anything.

arch/Kconfig validate_end: · line 844

Quoted 9 lines

arch/Kconfig +8 -0

+	ppt = find_probept(uproc, probept);
+	if (!ppt)
+		return 0;
+	get_probept(ppt);
+	if (utask) {
+		utask->active_ppt = ppt;
+		utask->state = UTASK_BP_HIT;
+	}

Right, so all this should die..

Obtain the address, lookup the vma, get the file, compute the offset inside that file -> {inode,offset}

Do the lookup in the global tree to obtain the uprobe, and possibly execute the probe handler right here in interrupt context, if it fails with -EFAULT, continue with the below:

arch/Kconfig validate_end: · line 847

Quoted 4 lines

arch/Kconfig +3 -0

+#ifdef CONFIG_ARCH_SUPPORTS_UPROBES
+	set_thread_flag(TIF_UPROBE);
+#endif

How can we ever get here if the architecture doesn't support uprobes?

Quoted 4 lines

arch/Kconfig +3 -0

+	return 1;
+}
+

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Srikar Dronamraju

TODO:
1. Allow multiple probes at a probepoint.
2. Booster probes.
3. Allow probes to be inherited across fork.

I wouldn't worry about that, focus on inode attached probes and you get fork for free.

I am working on the file based probing. It compiles but havent got it to test it yet, I can post the patch if you are interested. It should achieve similar to inode probing.

However I would have an issue with making inode based probing the default.
1. Making all probing based on inode can be a performance hog.

2. Since unlike kernel space, every process has a different space, so why would we have to insert breakpoints in each of its process space if we are not interested in them.

3. Ingo has a requirement for allowing normal users to use uprobes thro perf. When this feature gets implemented, we have to be careful about a normal users trying to just trace their application resulting in it hitting performance all other users.

For example: one user places a probe on /usr/lib/libc.so: malloc
- Another normal users looks at the current userspace probes and constructs a program that just does malloc/free just to degrade the performance of the system.

- user could be interested in just one process which could be calling malloc just 10 times. However during the same time there are 1000 processes which could all together call 100000 times during the same time.

Quoted 9 lines

include/linux/uprobes.h extern int post_xol(struct task_struct *tsk, · line 180

Quoted 7 lines

include/linux/uprobes.h +7 -0

+/* This is what the user supplies us. */
+struct uprobe {
+	/*
+	 * The pid of the probed process.  Currently, this can be the
+	 * thread ID (task->pid) of any active thread in the process.
+	 */
+	pid_t pid;

Would fail for pid namespaces I guess...

Can you explain a bit more?

Quoted 29 lines

Quoted 14 lines

include/linux/uprobes.h +12 -0

+	/* Location of the probepoint */
+	unsigned long vaddr;
+
+	/* Handler to run when the probepoint is hit */
+	void (*handler)(struct uprobe*, struct pt_regs*);
+};
 
 
+	/* breakpoint/XOL details */
+	struct user_bkpt user_bkpt;
+
+	/*
+	 * ppt goes in the uprobe_process->uprobe_table when registered --
+	 * even before the breakpoint has been inserted.

include/linux/uprobes.h extern int post_xol(struct task_struct *tsk, · line 244

Quoted 10 lines

include/linux/uprobes.h +10 -0

+	 */
+	struct list_head ut_node;
+
+	atomic_t refcount;
+
+	/* The parent uprobe_process */
+	struct uprobe_process *uproc;
+
+	struct uprobe *uprobe;
+};

So this thing is a link between the process and the probe, I'm not quite sure what you need the refcount for, it seems to me you can only have on of these per process/probe combination.

Quoted 4 lines

If you had used inode/offset based probes they would have been unique in the system and you could have had an {inode,offset} indexed global tree (or possibly a tree per inode, but that would mean adding to the inode structure, which I think is best avoided).

Unlike kernel probing, uprobes has a disadvantage.
Lets assume that the request for removing a probepoint when some of the threads have actually hit the probe. Because the handlers in uprobes can sleep, we cant remove the probepoint at the same time as the request for the removing the probe. This is where refcount steps in and helps us to decide when we can remove the probepoint. Even inoode based tracing or file based tracing would need it.

Quoted 19 lines

That would also reduce the mm state to purely the xol area, no need to manage this process to probe map.

Quoted 15 lines

include/linux/uprobes.h +15 -0

+enum uprobe_task_state {
+	UTASK_RUNNING,
+	UTASK_BP_HIT,
+	UTASK_SSTEP
+};
+
+/*
+ * uprobe_utask -- not a user-visible struct.
+ * Corresponds to a thread in a probed process.
+ * Guarded by uproc->mutex.
+ */
+struct uprobe_task {
+	struct user_bkpt_task_arch_info arch_info;
+
+	enum uprobe_task_state state;

Quoted 8 lines

include/linux/uprobes.h extern int post_xol(struct task_struct *tsk, · line 263

Quoted 3 lines

include/linux/uprobes.h +3 -0

+
+	struct uprobe_probept *active_ppt;
+};

I would be thinking you can obtain the active probe point from the address the task is stuck at and the state seems fairly redundant. Which leaves you with the arch state, which afaict is exactly as large as the pointer currently in the task struct.

Lets assume the thread is about to singlestep (or has singlestepped) So the instruction pointer is pointing to one of the slot (or it could be pointing to some other arbitrary address if it has singlestepped lets say a jump instruction).

We could get the active probe point from the address the task is stuck if the instruction pointers happens to be pointing to slots. But for that we would have to maintain a relation between slots and the current probepoint it is servicing. I think its easier to cache the active_probept then try to maintain this relationship. Moreover we are never certain in some cases where the underlying instruction happened to be a jump.

state is needed so that we know at do_notify_resume time whether the thread should singlestep or has already singlestep.
Do you see a way to determine in do_notify_resume if the thread is singlestepping or not. I dont think the instruction pointer can determine if we are about to singlestep or have already singlestepped.

Quoted 4 lines

Quoted 4 lines

kernel/uprobes.c +0 -0

@@ -741,6 +745,642 @@ validate_end:
 }
 /* end of slot allocation for XOL */
 

Quoted 12 lines

kernel/uprobes.c validate_end: · line 757

Quoted 10 lines

kernel/uprobes.c +10 -0

+
+struct notifier_block uprobes_exception_nb = {
+	.notifier_call = uprobes_exception_notify,
+	.priority = 0x7ffffff0,
+};
+
+typedef void (*uprobe_handler_t)(struct uprobe*, struct pt_regs*);
+
+/* Guards lookup, creation, and deletion of uproc. */
+static DEFINE_MUTEX(uprobe_mutex);

uproc is per mm, why a global mutex?

yes, uproc is per mm but gets created when the first probe on that process gets registered.
Do you have anyother lock that you have in mind?
I could have used the mmap_sem, but I am not sure if we should be taking a write lock on the mmap_sem.

Quoted 12 lines

Quoted 12 lines

kernel/uprobes.c +11 -0

+ */
+void uprobe_handle_fork(struct task_struct *child)
+{
+	struct uprobe_process *uproc;
+	struct uprobe_probept *ppt;
+	int ret;
+
+	uproc = current->mm->uproc;
+
+	/*
+	 * New process spawned by parent but not sharing the same mm.

Quoted 8 lines

kernel/uprobes.c validate_end: · line 1248

Quoted 5 lines

kernel/uprobes.c +5 -0

+	 * Remove the probepoints in the child's text.
+	 *
+	 * We also hold the uproc->mutex for the parent - so no
+	 * new uprobes will be registered 'til we return.
+	 */

The grand thing about not having any of this process state is that you dont need to clean it up either..

If we had done inode based tracing and a process that has breakpoints forks, wont we have to recreate the breakpoint metadata for the child process? I dont see the child calling mmap. When and how would be the probepoint data, user_bkpt data for the breakpoints, get replicated/linked to the child?

Quoted 20 lines

Quoted 7 lines

kernel/uprobes.c +7 -0

+	if (unlikely(!utask)) {
+		utask = add_utask(uproc);
+
+		/* Failed to allocate utask for the current task. */
+		BUG_ON(!utask);
+		probept = uprobes_get_bkpt_addr(regs);
+		ppt = find_probept(uproc, probept);

kernel/uprobes.c validate_end: · line 1297

Quoted 10 lines

kernel/uprobes.c +10 -0

+
+		/*
+		 * The probept was refcounted in uprobe_bkpt_notifier;
+		 * Hence it would be mysterious to miss ppt now
+		 */
+		WARN_ON(!ppt);
+		utask->active_ppt = ppt;
+		utask->state = UTASK_BP_HIT;
+	} else
+		ppt = utask->active_ppt;

You can replace this with:

Quoted 2 lines

addr = instruction_pointer(task_pt_regs(current)) -
ip_advancement_by_brkpt_insn;

uprobes_get_bkpt_addr does this exactly. However after we have singlestepped, we cant rely on this. Also how do we know if we have already singlestepped or not? After singlestepping we may still be pointing to some slot or the instruction pointer could be pointing something completely different.

Quoted 11 lines

kernel/uprobes.c validate_end: · line 1342

Quoted 8 lines

kernel/uprobes.c +8 -0

+	ppt = find_probept(uproc, probept);
+	if (!ppt)
+		return 0;
+	get_probept(ppt);
+	if (utask) {
+		utask->active_ppt = ppt;
+		utask->state = UTASK_BP_HIT;
+	}

Right, so all this should die..

Quoted 6 lines

Obtain the address, lookup the vma, get the file, compute the offset inside that file -> {inode,offset}

Do the lookup in the global tree to obtain the uprobe, and possibly execute the probe handler right here in interrupt context, if it fails with -EFAULT, continue with the below:

We dropped running the handler in interrupt context based on comments to my previous postings. It not only makes it complex at the time of handling the probe but we didnt see a difference when we compare the time it took to run the handler in task context to running the handler in interrupt context. I had even posted the timing info when register_uprobes was exported.

Quoted 4 lines

However I would have an issue with making inode based probing the default.
1. Making all probing based on inode can be a performance hog.

How so? You can add filters if you want.

Quoted 4 lines

2. Since unlike kernel space, every process has a different space, so why would we have to insert breakpoints in each of its process space if we are not interested in them.

You don't have to, but you can. The problem I have with this stuff is that it makes the pid thing a primary interface, whereas it should be one of many filter possibilities.

Quoted 6 lines

Quoted 4 lines

include/linux/uprobes.h extern int post_xol(struct task_struct *tsk, · line 180

Quoted 2 lines

include/linux/uprobes.h +2 -0

+	 */
+	pid_t pid;

Would fail for pid namespaces I guess...

Can you explain a bit more?

What pid namespace will you interpret that number in?

Quoted 8 lines

Unlike kernel probing, uprobes has a disadvantage.
Lets assume that the request for removing a probepoint when some of the threads have actually hit the probe. Because the handlers in uprobes can sleep, we cant remove the probepoint at the same time as the request for the removing the probe. This is where refcount steps in and helps us to decide when we can remove the probepoint. Even inoode based tracing or file based tracing would need it.

Stick the refcount in the actual struct uprobe instead?

Quoted 6 lines

state is needed so that we know at do_notify_resume time whether the thread should singlestep or has already singlestep.
Do you see a way to determine in do_notify_resume if the thread is singlestepping or not. I dont think the instruction pointer can determine if we are about to singlestep or have already singlestepped.

The to singlestep or not would be implied by the IP pointing to the start of a slot or not, but yes, I guess that as long as you do singlestep you need some state.. sucks though. Boosted probes are much nicer, they don't need that extra arch storage either, they can simply push/pop the register.

Quoted 11 lines

Quoted 5 lines

kernel/uprobes.c validate_end: · line 757

Quoted 3 lines

kernel/uprobes.c +3 -0

+
+/* Guards lookup, creation, and deletion of uproc. */
+static DEFINE_MUTEX(uprobe_mutex);

uproc is per mm, why a global mutex?

yes, uproc is per mm but gets created when the first probe on that process gets registered.
Do you have anyother lock that you have in mind?
I could have used the mmap_sem, but I am not sure if we should be taking a write lock on the mmap_sem.

No particular other lock in mind, you could cmpxchg the pointer if that's all you need it for. The problem is that if you want inode based probes without a filter this will become a rather hot lock on fork().

Quoted 7 lines

We dropped running the handler in interrupt context based on comments to my previous postings. It not only makes it complex at the time of handling the probe but we didnt see a difference when we compare the time it took to run the handler in task context to running the handler in interrupt context. I had even posted the timing info when register_uprobes was exported.

Is that because of the singlestep overhead? With boosted probes I would think it'd be much faster to take 1 trap, deal with it and continue execution, than to frob tons of kernel code in between.

A bit more about these filter thingies, add a method to struct uprobe, something like int uprobe::wants_probe(struct task_struct *p) and add a single bit to task_struct (there's a few bitfields with holes in there).

The on clone()/mmap() call the relevant wants_probe() methods, if one is true, set the task_struct::has_uprobe flag and install the probes.

If nothing in the process wants probing, you'll never install the probes and nothing ever triggers, of only one of many tasks in the process gets tagged, you'll have to look up the probe anyway to know where to continue, but you can avoid calling the handler.

Quoted 8 lines

Quoted 7 lines

Unlike kernel probing, uprobes has a disadvantage.
Lets assume that the request for removing a probepoint when some of the threads have actually hit the probe. Because the handlers in uprobes can sleep, we cant remove the probepoint at the same time as the request for the removing the probe. This is where refcount steps in and helps us to decide when we can remove the probepoint. Even inoode based tracing or file based tracing would need it.

Quoted 1 line

Stick the refcount in the actual struct uprobe instead?

What if the called does something like this when one or more threads are processing the breakpoint. unregister_uprobe(u);
kfree(u);

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines

However I would have an issue with making inode based probing the default.
1. Making all probing based on inode can be a performance hog.

Quoted 1 line

How so? You can add filters if you want.

The breakpoint exception and singlestep account for a substaintial time
of the uprobes probe handling. With increasing number of breakpoint hits and singlesteps, wouldnt the overall load increase?

Quoted 8 lines

Quoted 4 lines

2. Since unlike kernel space, every process has a different space, so why would we have to insert breakpoints in each of its process space if we are not interested in them.

You don't have to, but you can. The problem I have with this stuff is that it makes the pid thing a primary interface, whereas it should be one of many filter possibilities.

I think the otherway,
Why instrument a process and filter it out, if we are not interested in it. While instrumenting kernel, we dont have this flexibility. So having a pid based filter is the right thing to do for kernel based tracing.

If we can get the per process based tracing right, we can build higher lever stuff including the file based tracing easily.

Just to clarify, I am not looking at probing per task
but probing per process. So the pid field in uprobe refers to the tgid.

Quoted 9 lines · Srikar Dronamraju

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines

However I would have an issue with making inode based probing the default.
1. Making all probing based on inode can be a performance hog.

How so? You can add filters if you want.

Quoted 5 lines

The breakpoint exception and singlestep account for a substaintial time
of the uprobes probe handling. With increasing number of breakpoint hits and singlesteps, wouldnt the overall load increase?

You're really not getting it, are you? No, it would result in the exact same amount of actual breakpoints hit.

Quoted 16 lines

Quoted 9 lines

Quoted 5 lines

2. Since unlike kernel space, every process has a different space, so why would we have to insert breakpoints in each of its process space if we are not interested in them.

You don't have to, but you can. The problem I have with this stuff is that it makes the pid thing a primary interface, whereas it should be one of many filter possibilities.

I think the otherway,
Why instrument a process and filter it out, if we are not interested in it. While instrumenting kernel, we dont have this flexibility. So having a pid based filter is the right thing to do for kernel based tracing.

Quoted 2 lines

If we can get the per process based tracing right, we can build higher lever stuff including the file based tracing easily.

Creating inode based probes on top of pid based probes is terribly ugly.

Quoted 4 lines

Just to clarify, I am not looking at probing per task
but probing per process. So the pid field in uprobe refers to the tgid.

Urgh,.. I really oppose the whole pid-centric thing, if that means process wide and not per task its even worse.

Quoted 2 lines

You're really not getting it, are you? No, it would result in the exact same amount of actual breakpoints hit.

If there is just one instance of traced process for the inode then yes the number of breakpoints when traced with pid or based on inode would be the same. However if there are multiple instances of the traced process [example bash/zsh] (or the inode corresponds to a library that gets mapped into multiple processes example libc), and the user is interested in tracing just one instance of the process, then dont wont the inode based tracing amount to far more number of breakpoints hits?

Quoted 3 lines

multiple processes example libc), and the user is interested in tracing just one instance of the process, then dont wont the inode based tracing amount to far more number of breakpoints hits?

Not if your filter function works.

So let me try this again, (assumes boosted probes):

struct uprobe {
struct inode *inode; /* we hold a ref */
unsigned long offset;

int (*handler)(void); /* arguments.. ? */
int (*filter)(struct task_struct *);

int insn_size; /* size of */
char insn[MAX_INSN_SIZE]; /* the original insn */

int ret_addr_offset; /* return addr offset
in the slot */
char replacement[SLOT_SIZE]; /* replacement
instructions */

atomic_t ref; /* lifetime muck */
struct rcu_head rcu;
};

static struct {
	raw_spinlock_t	tree_lock;
	rb_root		tree;
} uprobes;

static void uprobes_add(struct uprobe *uprobe)

{
	/* add to uprobes.tree, sorted on inode:offset */
}

static void uprobes_del(struct uprobe *uprobe)

{
	/* delete from uprobes.tree */
}

static struct uprobe *
uprobes_find_get(struct address_space *mapping, unsigned long offset)

{
	unsigned long flags;
	struct uprobe *uprobe;

	raw_spin_lock_irqsave(&uprobes.treelock, flags);
	uprobe = find_in_tree(&uprobes.tree);

	if (!atomic_inc_not_zero(&uprobe->ref))
		uprobe = NULL;
	raw_spin_unlock_irqrestore(&uprobes.treelock, flags);

	return uprobe;
}

static void __uprobe_free(struct rcu_head *head)

{
	struct uprobe *uprobe = container_of(head, struct uprobe, rcu);

	kfree(uprobe);
}

static void put_uprobe(struct uprobe *uprobe)

{
	if (atomic_dec_and_test(&uprobe->ref))
		call_rcu(&uprobe->rcu, __uprobe_free);

}

static inline int valid_vma(struct vm_area_struct *vma)

{
	if (!vma->vm_file)
		return 0;

	if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED) ==
			    (VM_READ|VM_EXEC))
		return 1;

	return 0;
}

int register_uprobe(struct uprobe *uprobe)

{
	struct vm_area_struct *vma;

	inode_get(uprobe->inode);
	atomic_set(1, &uprobe->ref);

	uprobes_add(uprobe); /* add before the rmap walk, so that 
				new mmap()s will find it too */

	for_each_rmap_vma(vma, uprobe->inode->i_mapping) {

		struct mm_struct *mm = vma->vm_mm;
		int install_probe = 0;

		if (!valid_vma(vma))
			continue;

		for_each_task_in_process(p, mm->owner) {
			if (uprobe->filter(p)) {
				p->has_uprobe = 1;
				install_probe = 1;
			}
		}

		if (install_probe) {
			mm->has_uprobes = 1;
			frob_text(uprobe, mm);
		}
	}
}

void unregister_uprobe(struct uprobe *uprobe)

{
	/* pretty much the same, except restore the original text */
	put_uprobe(uprobe);
}

void uprobe_fork(struct task_struct *child)