Re: [PATCH v4 3.0-rc2-tip 4/22] 4: Uprobes: register/unregisterprobes.

[Srikar Dronamraju]

> >
> > +#ifdef CONFIG_UPROBES
> > +	unsigned long uprobes_vaddr;
> 
> Srikar, I know it is very easy to blame the patches ;) But why does this
> patch add mm->uprobes_vaddr ? Look, it is write-only, register/unregister
> do
> 
> 	mm->uprobes_vaddr = (unsigned long) vaddr;
> 
> and it is not used otherwise. It is not possible to understand its purpose

mm->uprobes_vaddr is used in helper routines insert(remove)_breakpoint
routines which are just stubs here. mm->uprobes_vaddr caches the vaddr
for subsequent use in insert_breakpoint.

I could have moved the mm->uprobes_vaddr to the 6th patch that
implemented the insert_breakpoint routine.  However at that time I felt
that people would comment back saying we do all the checks and get the
correct vaddr, but we dont cache it for subsequent use.

I will move adding the uprobes_vaddr initialization to the next patch.
Infact I might remove mm->uprobes_vaddr in the subsequent posting.

In one of the previous postings, I had the patches that used the helper
routines (like insert_breakpoint) first and then patches for wrapper
routines (like register/unregister) followed in the next patch. I was
told that it was tough to understand the context in which these helper
routines would be called. So I moved to having the wrapper routines with
stubs and implementing the stubs later.
 

> without reading the next patches. And the code above looks very strange,
> the next vma can overwrite uprobes_vaddr.

For this posting, handling two vmas for the same inode in the same mm
was a TODO. Since you and Peter have raised this I will handle it in the next posting. I will give a brief description of how I plan to implement this in my response to Peter's comments. Please do review and comment to it.

> 
> If possible, please try to re-split this series. If uprobes_vaddr is used
> in 6/22, then this patch should introduce this member. Note that this is
> only one particular example, there are a lot more.
> 
> > +int register_uprobe(struct inode *inode, loff_t offset,
> > +				struct uprobe_consumer *consumer)
> > +{
> > ...
> > +	mutex_lock(&mapping->i_mmap_mutex);
> > +	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, 0, 0) {
> > +		loff_t vaddr;
> > +		struct task_struct *tsk;
> > +
> > +		if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
> > +			continue;
> > +
> > +		mm = vma->vm_mm;
> > +		if (!valid_vma(vma)) {
> > +			mmput(mm);
> 
> This looks deadlockable. If mmput()->atomic_dec_and_test() succeeds
> unlink_file_vma() needs the same ->i_mmap_mutex, no?


okay, 

> 
> I think you can simply remove mmput(). Why do you increment ->mm_users
> in advance? I think you can do this right before list_add(), after all
> valid_vma/etc checks.

Okay, will modify as suggested.

> 
> > +		vaddr = vma->vm_start + offset;
> > +		vaddr -= vma->vm_pgoff << PAGE_SHIFT;
> > +		if (vaddr < vma->vm_start || vaddr > vma->vm_end) {
> > +			/* Not in this vma */
> > +			mmput(mm);
> > +			continue;
> > +		}
> 

> Not sure that "Not in this vma" is possible if we pass the correct pgoff
> to vma_prio_tree_foreach()... but OK, I forgot everything I knew about
> vma prio_tree.
> 

I was asked what if the arithmetic to arrive at vaddr would end up not
being in the range.

> So, we verified that vaddr is valid. Then,
> 
> > +		tsk = get_mm_owner(mm);
> > +		if (tsk && vaddr > TASK_SIZE_OF(tsk)) {
> 
> how it it possible to map ->vm_file above TASK_SIZE ?

Same as above. I will do a rethink on both of these checks.

> 
> And why do you need get/put_task_struct? You could simply read
> TASK_SIZE_OF(tsk) under rcu_read_lock.

Yes, for register/unregister case I could have just done the check under
rcu_read_lock instead of doing a get/put_task_struct. Since I needed
get_mm_owner() for insert/remove_breakpoint, I thought I will reuse it
here. 

> 
> > +void unregister_uprobe(struct inode *inode, loff_t offset,
> > +				struct uprobe_consumer *consumer)
> > +{
> > ...
> > +
> > +	mutex_lock(&mapping->i_mmap_mutex);
> > +	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, 0, 0) {
> > +		struct task_struct *tsk;
> > +
> > +		if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
> > +			continue;
> > +
> > +		mm = vma->vm_mm;
> > +
> > +		if (!atomic_read(&mm->uprobes_count)) {
> > +			mmput(mm);
> 
> Again, mmput() doesn't look safe.


Okay, I will increment the mm_users while adding to the list.

> 
> > +	list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list)
> > +		remove_breakpoint(mm, uprobe);
> 
> What if the application, say, unmaps the vma with bkpt before
> unregister_uprobe() ? Or it can do mprotect(PROT_WRITE), then valid_vma()
> fails. Probably this is fine, but mm->uprobes_count becomes wrong, no?

Okay, will add a hook in unmap to keep the mm->uprobes_count sane.

> 
> Oleg.
> 

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