uprobes: Breakpoint insertion/removal in user space applications.
in [Kernel]
|
Prev: [PATCH 1/8] v3 Move the find_memory_block() routine up
Next: linux-next: manual merge of the workqueues tree with the tip tree
From: Christoph Hellwig on 20 Jul 2010 00:30 > +struct user_bkpt_arch_info { > + void (*set_ip)(struct pt_regs *regs, unsigned long vaddr); > + int (*validate_address)(struct task_struct *tsk, unsigned long vaddr); > + int (*read_opcode)(struct task_struct *tsk, unsigned long vaddr, > + user_bkpt_opcode_t *opcode); > + int (*set_bkpt)(struct task_struct *tsk, > + struct user_bkpt *user_bkpt); > + int (*set_orig_insn)(struct task_struct *tsk, > + struct user_bkpt *user_bkpt, bool check); > + bool (*is_bkpt_insn)(struct user_bkpt *user_bkpt); > + int (*analyze_insn)(struct task_struct *tsk, > + struct user_bkpt *user_bkpt); > + int (*pre_xol)(struct task_struct *tsk, > + struct user_bkpt *user_bkpt, > + struct user_bkpt_task_arch_info *tskinfo, > + struct pt_regs *regs); > + int (*post_xol)(struct task_struct *tsk, > + struct user_bkpt *user_bkpt, > + struct user_bkpt_task_arch_info *tskinfo, > + struct pt_regs *regs); > +}; Just wondering why these are function pointers. Do we exepect an architecture to provide different versions of these for say 32 vs 64-bit binaries? If not just making these arch provided helpers might be a lot simpler. Especially in the current version where only very few of these are overriden by the architecture at all. > +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) > +{ > + unsigned long nleft; > + > + if (tsk == current) { > + nleft = copy_to_user(vaddr, kbuf, nbytes); > + return nbytes - nleft; > + } else > + return access_process_vm(tsk, (unsigned long) vaddr, > + (void *) kbuf, nbytes, 1); > +} Any reason for the naming mismatch between _read_vm and _write_data? Also I wonder if the optimization for tsk == current should be folded directly into access_process_vm instead of adding these wrappers. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Srikar Dronamraju on 20 Jul 2010 03:30 * Christoph Hellwig <hch(a)infradead.org> [2010-07-20 00:28:14]: > > +struct user_bkpt_arch_info { > > + void (*set_ip)(struct pt_regs *regs, unsigned long vaddr); > > + int (*validate_address)(struct task_struct *tsk, unsigned long vaddr); > > + int (*read_opcode)(struct task_struct *tsk, unsigned long vaddr, > > + user_bkpt_opcode_t *opcode); > > + int (*set_bkpt)(struct task_struct *tsk, > > + struct user_bkpt *user_bkpt); > > + int (*set_orig_insn)(struct task_struct *tsk, > > + struct user_bkpt *user_bkpt, bool check); > > + bool (*is_bkpt_insn)(struct user_bkpt *user_bkpt); > > + int (*analyze_insn)(struct task_struct *tsk, > > + struct user_bkpt *user_bkpt); > > + int (*pre_xol)(struct task_struct *tsk, > > + struct user_bkpt *user_bkpt, > > + struct user_bkpt_task_arch_info *tskinfo, > > + struct pt_regs *regs); > > + int (*post_xol)(struct task_struct *tsk, > > + struct user_bkpt *user_bkpt, > > + struct user_bkpt_task_arch_info *tskinfo, > > + struct pt_regs *regs); > > +}; > > Just wondering why these are function pointers. Do we exepect an > architecture to provide different versions of these for say 32 vs 64-bit > binaries? If not just making these arch provided helpers might be a lot > simpler. Especially in the current version where only very few of these > are overriden by the architecture at all. > > > +unsigned long uprobes_read_vm(struct task_struct *tsk, void __user *vaddr, > > + void *kbuf, unsigned long nbytes) Some of these functions are purely optional example being validate_address. Some of these functions need not be defined by the architecture in which case we default to the functions defined in common code. examples being: read_opcode, set_bkpt, and set_orig_insn. Some of these functions are architecture mode specific, for example there is a architecture specific pre_xol needed for x86_64. However generic pre_xol for x86_32 would suffice for x86_32. Some of these functions need to be mandatorily defined by the architecture. example being set_ip and analyze_insn. Apart from the above flexibilities and enforcements that we can make when we use function pointers, its would be handy to incorporate more enhancements like return probes and booster. > > +{ > > + 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) > > +{ > > + unsigned long nleft; > > + > > + if (tsk == current) { > > + nleft = copy_to_user(vaddr, kbuf, nbytes); > > + return nbytes - nleft; > > + } else > > + return access_process_vm(tsk, (unsigned long) vaddr, > > + (void *) kbuf, nbytes, 1); > > +} > > Any reason for the naming mismatch between _read_vm and _write_data? The naming mismatch was on purpose, we wanted to mention that write_data cannot be used with code sections unlike read_vm which can be used to read code section. > > Also I wonder if the optimization for tsk == current should be folded > directly into access_process_vm instead of adding these wrappers. > One reason why we dont want to move this optimization as is into access_process_vm is we dont want to do a copy_to_user on a code section. So that would mean another check to determine if its a code section before we do the optimization. However there could other reasons why we shouldnt be doing this optimization. Do you still think we should still be pursuing with the optimization? -- Thanks and Regards Srikar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 4 Aug 2010 08:10 On Tue, 2010-07-20 at 12:52 +0530, Srikar Dronamraju wrote: > > Just wondering why these are function pointers. Do we exepect an > > architecture to provide different versions of these for say 32 vs 64-bit > > binaries? If not just making these arch provided helpers might be a lot > > simpler. Especially in the current version where only very few of these > > are overriden by the architecture at all. > > > > > +unsigned long uprobes_read_vm(struct task_struct *tsk, void __user *vaddr, > > > + void *kbuf, unsigned long nbytes) > > > Some of these functions are purely optional example being > validate_address. > > Some of these functions need not be defined by the architecture in > which case we default to the functions defined in common code. > examples being: read_opcode, set_bkpt, and set_orig_insn. > > Some of these functions are architecture mode specific, for example > there is a architecture specific pre_xol needed for x86_64. However > generic pre_xol for x86_32 would suffice for x86_32. > > Some of these functions need to be mandatorily defined by the > architecture. example being set_ip and analyze_insn. > > Apart from the above flexibilities and enforcements that we can make > when we use function pointers, its would be handy to incorporate > more enhancements like return probes and booster. Still not sure why you're using this vector though, why not use weak function for optionals and defaults and no implementation for mandatory functions (and if the implementations fails to provide it, that will result in a link error). Are there likely to be multiple different versions of this method vector around on a running kernel? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Srikar Dronamraju on 4 Aug 2010 09:00 * Peter Zijlstra <peterz(a)infradead.org> [2010-08-04 14:05:28]: > On Tue, 2010-07-20 at 12:52 +0530, Srikar Dronamraju wrote: > > > Just wondering why these are function pointers. Do we exepect an > > > architecture to provide different versions of these for say 32 vs 64-bit > > > binaries? If not just making these arch provided helpers might be a lot > > > simpler. Especially in the current version where only very few of these > > > are overriden by the architecture at all. > > > > > > > Some of these functions are purely optional example being > > validate_address. > > > > Some of these functions need not be defined by the architecture in > > which case we default to the functions defined in common code. > > examples being: read_opcode, set_bkpt, and set_orig_insn. > > > > Some of these functions are architecture mode specific, for example > > there is a architecture specific pre_xol needed for x86_64. However > > generic pre_xol for x86_32 would suffice for x86_32. > > > > Some of these functions need to be mandatorily defined by the > > architecture. example being set_ip and analyze_insn. > > > > Apart from the above flexibilities and enforcements that we can make > > when we use function pointers, its would be handy to incorporate > > more enhancements like return probes and booster. > > Still not sure why you're using this vector though, why not use weak > function for optionals and defaults and no implementation for mandatory > functions (and if the implementations fails to provide it, that will > result in a link error). Yes, we can certainly use weak functions instead of pointers. One another reason why we had these as function pointers in a structure was that it would easy be for a person porting uprobes to a new architecture. i.e person porting to a new architecture knows in one place(structure) which all functions need to be provided. However I would go with your suggestion and make the changes to use weak functions in the next version of the patchset. > > Are there likely to be multiple different versions of this method vector > around on a running kernel? No for a running kernel, there will be only one method vector. Also wanted to check with if you had tried perf probes and had comments/suggestions on any of the other patches in the patchset. -- Thanks and Regards Srikar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 4 Aug 2010 09:10
On Wed, 2010-08-04 at 18:18 +0530, Srikar Dronamraju wrote: > > Also wanted to check with if you had tried perf probes and had > comments/suggestions on any of the other patches in the patchset. Just got back from holidays and am trying to make sense of the inbox, I'll hopefully get around to actually reading the patches 'soon' ;-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |