* Christoph Hellwig <hch@infradead.org> [2010-07-20 00:28:14]:

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

quoted text
> > +{
> > +	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.

quoted text
> 
> 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
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/