Messages in this thread | | | Subject | Re: [PATCH v3 2.6.39-rc1-tip 8/26] 8: uprobes: store/restore original instruction. | From | Peter Zijlstra <> | Date | Mon, 18 Apr 2011 18:12:47 +0200 |
| |
On Fri, 2011-04-01 at 20:03 +0530, Srikar Dronamraju wrote:
> +static int __copy_insn(struct address_space *mapping, char *insn, > + unsigned long nbytes, unsigned long offset) > +{ > + struct page *page; > + void *vaddr; > + unsigned long off1; > + loff_t idx; > + > + idx = offset >> PAGE_CACHE_SHIFT; > + off1 = offset &= ~PAGE_MASK; > + page = grab_cache_page(mapping, (unsigned long)idx);
What if the page wasn't present due to being swapped out?
> + if (!page) > + return -ENOMEM; > + > + vaddr = kmap_atomic(page, KM_USER0); > + memcpy(insn, vaddr + off1, nbytes); > + kunmap_atomic(vaddr, KM_USER0); > + unlock_page(page); > + page_cache_release(page); > + return 0; > +} > + > +static int copy_insn(struct uprobe *uprobe, unsigned long addr) > +{ > + struct address_space *mapping; > + int bytes; > + unsigned long nbytes; > + > + addr &= ~PAGE_MASK; > + nbytes = PAGE_SIZE - addr; > + mapping = uprobe->inode->i_mapping; > + > + /* Instruction at end of binary; copy only available bytes */ > + if (uprobe->offset + MAX_UINSN_BYTES > uprobe->inode->i_size) > + bytes = uprobe->inode->i_size - uprobe->offset; > + else > + bytes = MAX_UINSN_BYTES; > + > + /* Instruction at the page-boundary; copy bytes in second page */ > + if (nbytes < bytes) { > + if (__copy_insn(mapping, uprobe->insn + nbytes, > + bytes - nbytes, uprobe->offset + nbytes)) > + return -ENOMEM; > + bytes = nbytes; > + } > + return __copy_insn(mapping, uprobe->insn, bytes, uprobe->offset); > +}
This all made me think why implement read_opcode() again.. I know its all slightly different, but still.
> +static struct task_struct *uprobes_get_mm_owner(struct mm_struct *mm) > +{ > + struct task_struct *tsk; > + > + rcu_read_lock(); > + tsk = rcu_dereference(mm->owner); > + if (tsk) > + get_task_struct(tsk); > + rcu_read_unlock(); > + return tsk; > +}
Naming is somewhat inconsistent, most of your functions have the _uprobe postfix and now its a uprobes_ prefix all of a sudden.
> static int install_uprobe(struct mm_struct *mm, struct uprobe *uprobe) > { > - int ret = 0; > + struct task_struct *tsk = uprobes_get_mm_owner(mm); > + int ret; > > - /*TODO: install breakpoint */ > - if (!ret) > + if (!tsk) /* task is probably exiting; bail-out */ > + return -ESRCH; > + > + if (!uprobe->copy) { > + ret = copy_insn(uprobe, mm->uprobes_vaddr); > + if (ret) > + goto put_return; > + if (is_bkpt_insn(uprobe->insn)) { > + print_insert_fail(tsk, mm->uprobes_vaddr, > + "breakpoint instruction already exists"); > + ret = -EEXIST; > + goto put_return; > + } > + ret = analyze_insn(tsk, uprobe); > + if (ret) { > + print_insert_fail(tsk, mm->uprobes_vaddr, > + "instruction type cannot be probed"); > + goto put_return; > + }
If you want to expose this functionality to !root users printing stuff to dmesg like that isn't a good idea.
> + uprobe->copy = 1; > + } > + > + ret = set_bkpt(tsk, uprobe, mm->uprobes_vaddr); > + if (ret < 0) > + print_insert_fail(tsk, mm->uprobes_vaddr, > + "failed to insert bkpt instruction"); > + else > atomic_inc(&mm->uprobes_count); > + > +put_return: > + put_task_struct(tsk); > return ret; > }
| |