Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-25 13:15:45]:

> > +
> > +       if (atomic_read(&uprobe->ref) == 1) {
> > +               synchronize_sched();
> > +               rb_erase(&uprobe->rb_node, &uprobes_tree);
> 
> How is that safe without holding the treelock?

Right, 
Something like this should be good enuf right?

if (atomic_read(&uprobe->ref) == 1) {
	synchronize_sched();
	spin_lock_irqsave(&treelock, flags);
	rb_erase(&uprobe->rb_node, &uprobes_tree);
	spin_lock_irqrestore(&treelock, flags);
	iput(uprobe->inode);
}
	
-- 
Thanks and Regards
Srikar

PS: Last time I had goofed up with Linux-mm mailing alias. 
Hopefully this time it goes to the right list.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Wed, 2011-01-26 at 13:17 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@infradead.org> [2011-01-25 13:15:45]:
> 
> > > +
> > > +       if (atomic_read(&uprobe->ref) == 1) {
> > > +               synchronize_sched();
> > > +               rb_erase(&uprobe->rb_node, &uprobes_tree);
> > 
> > How is that safe without holding the treelock?
> 
> Right, 
> Something like this should be good enuf right?
> 
> if (atomic_read(&uprobe->ref) == 1) {
> 	synchronize_sched();
> 	spin_lock_irqsave(&treelock, flags);
> 	rb_erase(&uprobe->rb_node, &uprobes_tree);
> 	spin_lock_irqrestore(&treelock, flags);
> 	iput(uprobe->inode);
> }
> 	

How is the atomic_read() not racy with a future increment, and what is
that synchronize_sched() thing for?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

> > +
> > +               list_add(&mm->uprobes_list, &tmp_list);
> > +               mm->uprobes_vaddr = vma->vm_start + offset;
> > +       }
> > +       spin_unlock(&mapping->i_mmap_lock);
> 
> Both this and unregister are racy, what is to say:
>  - the vma didn't get removed from the mm
>  - no new matching vma got added
> 

register_uprobe, unregister_uprobe, uprobe_mmap are all synchronized by
uprobes_mutex. So I dont see one unregister_uprobe getting thro when
another register_uprobe is working with a vma.

If I am missing something elementary, please explain a bit more.

> > +       if (list_empty(&tmp_list)) {
> > +               ret = 0;
> > +               goto consumers_add;
> > +       }
> > +       list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list) {
> > +               if (!install_uprobe(mm, uprobe))
> > +                       ret = 0;
> > +               list_del(&mm->uprobes_list);
> > +               mmput(mm);
> > +       }
> > +
> > +consumers_add:
> > +       add_consumer(uprobe, consumer);
> > +       mutex_unlock(&uprobes_mutex);
> > +       put_uprobe(uprobe);
> > +       return ret;
> > +}
> > + 

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Wed, 2011-01-26 at 13:25 +0530, Srikar Dronamraju wrote:
> 
> > > +
> > > +               list_add(&mm->uprobes_list, &tmp_list);
> > > +               mm->uprobes_vaddr = vma->vm_start + offset;
> > > +       }
> > > +       spin_unlock(&mapping->i_mmap_lock);
> > 
> > Both this and unregister are racy, what is to say:
> >  - the vma didn't get removed from the mm
> >  - no new matching vma got added
> > 
> 
> register_uprobe, unregister_uprobe, uprobe_mmap are all synchronized by
> uprobes_mutex. So I dont see one unregister_uprobe getting thro when
> another register_uprobe is working with a vma.
> 
> If I am missing something elementary, please explain a bit more.

afaict you're not holding the mmap_sem, so userspace can simply unmap
the vma.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 11:11:48]:

> On Wed, 2011-01-26 at 13:25 +0530, Srikar Dronamraju wrote:
> > 
> > > > +
> > > > +               list_add(&mm->uprobes_list, &tmp_list);
> > > > +               mm->uprobes_vaddr = vma->vm_start + offset;
> > > > +       }
> > > > +       spin_unlock(&mapping->i_mmap_lock);
> > > 
> > > Both this and unregister are racy, what is to say:
> > >  - the vma didn't get removed from the mm
> > >  - no new matching vma got added
> > > 
> > 
> > register_uprobe, unregister_uprobe, uprobe_mmap are all synchronized by
> > uprobes_mutex. So I dont see one unregister_uprobe getting thro when
> > another register_uprobe is working with a vma.
> > 
> > If I am missing something elementary, please explain a bit more.
> 
> afaict you're not holding the mmap_sem, so userspace can simply unmap
> the vma.

When we do the actual insert/remove of the breakpoint we hold the
mmap_sem. During the actual insertion/removal, if the vma for the
specific inode is not found, we just come out without doing the
actual insertion/deletion.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Wed, 2011-01-26 at 21:00 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@infradead.org> [2011-01-26 11:11:48]:
> 
> > On Wed, 2011-01-26 at 13:25 +0530, Srikar Dronamraju wrote:
> > > 
> > > > > +
> > > > > +               list_add(&mm->uprobes_list, &tmp_list);
> > > > > +               mm->uprobes_vaddr = vma->vm_start + offset;
> > > > > +       }
> > > > > +       spin_unlock(&mapping->i_mmap_lock);
> > > > 
> > > > Both this and unregister are racy, what is to say:
> > > >  - the vma didn't get removed from the mm
> > > >  - no new matching vma got added
> > > > 
> > > 
> > > register_uprobe, unregister_uprobe, uprobe_mmap are all synchronized by
> > > uprobes_mutex. So I dont see one unregister_uprobe getting thro when
> > > another register_uprobe is working with a vma.
> > > 
> > > If I am missing something elementary, please explain a bit more.
> > 
> > afaict you're not holding the mmap_sem, so userspace can simply unmap
> > the vma.
> 
> When we do the actual insert/remove of the breakpoint we hold the
> mmap_sem. During the actual insertion/removal, if the vma for the
> specific inode is not found, we just come out without doing the
> actual insertion/deletion.

Right, but then install_uprobe() should:

 - lookup the vma relating to the address you stored,
 - validate that the vma is indeed a map of the right inode
 - validate that the offset of the probe corresponds with the stored
address

Otherwise you can race with unmap/map and end up installing the probe in
a random location.

Also, I think the whole thing goes funny if someone maps the same text
twice ;-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 16:45:56]:

> On Wed, 2011-01-26 at 21:00 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <peterz@infradead.org> [2011-01-26 11:11:48]:
> > 
> > > On Wed, 2011-01-26 at 13:25 +0530, Srikar Dronamraju wrote:
> > > > 
> > > > > > +
> > > > > > +               list_add(&mm->uprobes_list, &tmp_list);
> > > > > > +               mm->uprobes_vaddr = vma->vm_start + offset;
> > > > > > +       }
> > > > > > +       spin_unlock(&mapping->i_mmap_lock);
> > > > > 
> > > > > Both this and unregister are racy, what is to say:
> > > > >  - the vma didn't get removed from the mm
> > > > >  - no new matching vma got added
> > > > > 
> > > > 
> > > > register_uprobe, unregister_uprobe, uprobe_mmap are all synchronized by
> > > > uprobes_mutex. So I dont see one unregister_uprobe getting thro when
> > > > another register_uprobe is working with a vma.
> > > > 
> > > > If I am missing something elementary, please explain a bit more.
> > > 
> > > afaict you're not holding the mmap_sem, so userspace can simply unmap
> > > the vma.
> > 
> > When we do the actual insert/remove of the breakpoint we hold the
> > mmap_sem. During the actual insertion/removal, if the vma for the
> > specific inode is not found, we just come out without doing the
> > actual insertion/deletion.
> 
> Right, but then install_uprobe() should:
> 
>  - lookup the vma relating to the address you stored,

We already do this thro get_user_pages in write_opcode().

>  - validate that the vma is indeed a map of the right inode

We can add a check in write_opcode( we need to pass the inode to
write_opcode).

>  - validate that the offset of the probe corresponds with the stored
> address

I am not clear on this. We would have derived the address from the
offset. So is that we check for
 (vaddr == vma->vm_start + uprobe->offset)

> 
> Otherwise you can race with unmap/map and end up installing the probe in
> a random location.
> 
> Also, I think the whole thing goes funny if someone maps the same text
> twice ;-)

I am not sure if we can map the same text twice. If something like
this is possible then we would have 2 addresses for each function.
So how does the linker know which address to jump to out of the 2 or
multiple matching addresses. What would be the usecases for same
text being mapped multiple times and both being executable?

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Wed, 2011-01-26 at 22:26 +0530, Srikar Dronamraju wrote:

> >  - lookup the vma relating to the address you stored,
> 
> We already do this thro get_user_pages in write_opcode().

Ah, I didn't read that far..

> >  - validate that the vma is indeed a map of the right inode
> 
> We can add a check in write_opcode( we need to pass the inode to
> write_opcode).

sure..

> >  - validate that the offset of the probe corresponds with the stored
> > address
> 
> I am not clear on this. We would have derived the address from the
> offset. So is that we check for
>  (vaddr == vma->vm_start + uprobe->offset)

Sure, but the vma might have changed since you computed the offset -)

> > 
> > Otherwise you can race with unmap/map and end up installing the probe in
> > a random location.
> > 
> > Also, I think the whole thing goes funny if someone maps the same text
> > twice ;-)
> 
> I am not sure if we can map the same text twice. If something like
> this is possible then we would have 2 addresses for each function.
> So how does the linker know which address to jump to out of the 2 or
> multiple matching addresses. What would be the usecases for same
> text being mapped multiple times and both being executable?

You can, if only to wreck your thing, you can call mmap() as often as
you like (until your virtual memory space runs out) and get many many
mapping of the same file.

It doesn't need to make sense to the linker, all it needs to do is
confuse your code ;-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 18:12:29]:

> On Wed, 2011-01-26 at 22:26 +0530, Srikar Dronamraju wrote:
> 
> > >  - lookup the vma relating to the address you stored,
> > 
> > We already do this thro get_user_pages in write_opcode().
> 
> Ah, I didn't read that far..
> 
> > >  - validate that the vma is indeed a map of the right inode
> > 
> > We can add a check in write_opcode( we need to pass the inode to
> > write_opcode).
> 
> sure..
> 
> > >  - validate that the offset of the probe corresponds with the stored
> > > address
> > 
> > I am not clear on this. We would have derived the address from the
> > offset. So is that we check for
> >  (vaddr == vma->vm_start + uprobe->offset)
> 
> Sure, but the vma might have changed since you computed the offset -)

If the vma has changed then it would fail the 2nd validation i.e vma
corresponds to the uprobe inode right. If the vma was unmapped and
mapped back at the same place, then I guess we are okay to probe.

> 
> > > 
> > > Otherwise you can race with unmap/map and end up installing the probe in
> > > a random location.
> > > 
> > > Also, I think the whole thing goes funny if someone maps the same text
> > > twice ;-)
> > 
> > I am not sure if we can map the same text twice. If something like
> > this is possible then we would have 2 addresses for each function.
> > So how does the linker know which address to jump to out of the 2 or
> > multiple matching addresses. What would be the usecases for same
> > text being mapped multiple times and both being executable?
> 
> You can, if only to wreck your thing, you can call mmap() as often as
> you like (until your virtual memory space runs out) and get many many
> mapping of the same file.
> 
> It doesn't need to make sense to the linker, all it needs to do is
> confuse your code ;-)

Currently if there are multiple mappings of the same executable
code, only one mapped area would have the breakpoint inserted.

If the code were to execute from some other mapping, then it would
work as if there are no probes.  However if the code from the
mapping that had the breakpoint executes then we would see the
probes.

If we want to insert breakpoints in each of the maps then we
would have to extend mm->uprobes_vaddr.

Do you have any other ideas to tackle this?
Infact do you think we should be handling this case?

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-01-27 at 15:31 +0530, Srikar Dronamraju wrote:
> > > >  - validate that the vma is indeed a map of the right inode
> > > 
> > > We can add a check in write_opcode( we need to pass the inode to
> > > write_opcode).
> > 
> > sure..
> > 
> > > >  - validate that the offset of the probe corresponds with the stored
> > > > address
> > > 
> > > I am not clear on this. We would have derived the address from the
> > > offset. So is that we check for
> > >  (vaddr == vma->vm_start + uprobe->offset)
> > 
> > Sure, but the vma might have changed since you computed the offset -)
> 
> If the vma has changed then it would fail the 2nd validation i.e vma
> corresponds to the uprobe inode right. If the vma was unmapped and
> mapped back at the same place, then I guess we are okay to probe.

It can be unmapped and mapped back slightly different. A map of the same
file doesn't need to mean its in the exact same location or has the
exact same pgoffset.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-27 11:23:37]:

> On Thu, 2011-01-27 at 15:31 +0530, Srikar Dronamraju wrote:
> > > > >  - validate that the vma is indeed a map of the right inode
> > > > 
> > > > We can add a check in write_opcode( we need to pass the inode to
> > > > write_opcode).
> > > 
> > > sure..
> > > 
> > > > >  - validate that the offset of the probe corresponds with the stored
> > > > > address
> > > > 
> > > > I am not clear on this. We would have derived the address from the
> > > > offset. So is that we check for
> > > >  (vaddr == vma->vm_start + uprobe->offset)
> > > 
> > > Sure, but the vma might have changed since you computed the offset -)
> > 
> > If the vma has changed then it would fail the 2nd validation i.e vma
> > corresponds to the uprobe inode right. If the vma was unmapped and
> > mapped back at the same place, then I guess we are okay to probe.
> 
> It can be unmapped and mapped back slightly different. A map of the same
> file doesn't need to mean its in the exact same location or has the
> exact same pgoffset.
> 
> 

If its not at the exact same location, then our third validation of
checking that (vaddr == vma->vm_start + uprobe->offset)  should fail
right?

Also should it be (vaddr == uprobe->offset + vma->vm_start -
vma->pgoff << PAGE_SHIFT) ?

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-01-27 at 15:55 +0530, Srikar Dronamraju wrote:
> 
> 
> If its not at the exact same location, then our third validation of
> checking that (vaddr == vma->vm_start + uprobe->offset)  should fail
> right?
> 
> Also should it be (vaddr == uprobe->offset + vma->vm_start -
> vma->pgoff << PAGE_SHIFT) ?

Yeah, although I just realized that ->offset should be a u64, since
pgoff is a unsigned long, we can have files up to 44 bit (assuming the
page-size is 12bits).

But yes, this matches the validation I mentioned.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-01-27 at 15:31 +0530, Srikar Dronamraju wrote:
> 
> > You can, if only to wreck your thing, you can call mmap() as often as
> > you like (until your virtual memory space runs out) and get many many
> > mapping of the same file.
> > 
> > It doesn't need to make sense to the linker, all it needs to do is
> > confuse your code ;-)
> 
> Currently if there are multiple mappings of the same executable
> code, only one mapped area would have the breakpoint inserted.

Right, so you could use it to make debugging harder..

> If the code were to execute from some other mapping, then it would
> work as if there are no probes.  However if the code from the
> mapping that had the breakpoint executes then we would see the
> probes.
> 
> If we want to insert breakpoints in each of the maps then we
> would have to extend mm->uprobes_vaddr.
> 
> Do you have any other ideas to tackle this?

Supposing I can get my preemptible mmu patches anywhere.. you could
simply call install_uprobe() while holding the i_mmap_mutex ;-)

> Infact do you think we should be handling this case?

I'm really not sure how often this would happen, but dealing with it
sure makes me feel better..

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 4/20] 4: uprobes: Adding and remove a uprobe in a rb tree.

[Srikar Dronamraju]

> > +       spin_lock_irqsave(&treelock, flags);
> > +       while (*p) {
> > +               parent = *p;
> > +               u = rb_entry(parent, struct uprobe, rb_node);
> > +               if (u->inode > uprobe->inode)
> > +                       p = &(*p)->rb_left;
> > +               else if (u->inode < uprobe->inode)
> > +                       p = &(*p)->rb_right;
> > +               else {
> > +                       if (u->offset > uprobe->offset)
> > +                               p = &(*p)->rb_left;
> > +                       else if (u->offset < uprobe->offset)
> > +                               p = &(*p)->rb_right;
> > +                       else {
> > +                               atomic_inc(&u->ref);
> 
> If the lookup can find a 'dead' entry, then why can't we here?
> 

If a new user of a uprobe comes up as when the last registered user was
removing the uprobe, we keep the uprobe entry till the new user
loses interest in that uprobe.

> > +                               goto unlock_return;
> > +                       }
> > +               }
> > +       }
> > +       u = NULL;
> > +       rb_link_node(&uprobe->rb_node, parent, p);
> > +       rb_insert_color(&uprobe->rb_node, &uprobes_tree);
> > +       atomic_set(&uprobe->ref, 2);
> > +
> > +unlock_return:
> > +       spin_unlock_irqrestore(&treelock, flags);
> > +       return u;
> > +} 
> 
> It would be nice if you could merge the find and 'acquire' thing, the
> lookup is basically the same in both cases.
> 
> Also, I'm not quite sure on the name of that last function, its not a
> strict insert and what's the trailing _rb_node about? That lookup isn't
> called find_uprobe_rb_node() either is it?

Since we already have a install_uprobe, register_uprobe, I thought
insert_uprobe_rb_node would give context to that function that it was
only inserting an rb_node but not installing the actual breakpoint.
I am okay to rename it to insert_uprobe(). 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 14/20] 14: uprobes: Handing int3 and singlestep exception.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-25 14:56:19]:

> On Thu, 2010-12-16 at 15:29 +0530, Srikar Dronamraju wrote:
> > +               down_read(&mm->mmap_sem);
> > +               for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > +                       if (!valid_vma(vma))
> > +                               continue;
> > +                       if (probept < vma->vm_start || probept > vma->vm_end)
> > +                               continue;
> > +                       u = find_uprobe(vma->vm_file->f_mapping->host,
> > +                                       probept - vma->vm_start);
> > +                       if (u)
> > +                               break;
> > +               }
> > +               up_read(&mm->mmap_sem); 
> 
> One has to ask, what's wrong with find_vma() ?

Are you looking for something like this.

       down_read(&mm->mmap_sem);
	for (vma = find_vma(mm, probept); ; vma = vma->vm_next) {
	       if (!valid_vma(vma))
		       continue;
	       u = find_uprobe(vma->vm_file->f_mapping->host,
			       probept - vma->vm_start);
	       if (u)
		       break;
       }
       up_read(&mm->mmap_sem); 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 14/20] 14: uprobes: Handing int3 and singlestep exception.

[Peter Zijlstra]

On Wed, 2011-01-26 at 14:22 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@infradead.org> [2011-01-25 14:56:19]:
> 
> > On Thu, 2010-12-16 at 15:29 +0530, Srikar Dronamraju wrote:
> > > +               down_read(&mm->mmap_sem);
> > > +               for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > > +                       if (!valid_vma(vma))
> > > +                               continue;
> > > +                       if (probept < vma->vm_start || probept > vma->vm_end)
> > > +                               continue;
> > > +                       u = find_uprobe(vma->vm_file->f_mapping->host,
> > > +                                       probept - vma->vm_start);
> > > +                       if (u)
> > > +                               break;
> > > +               }
> > > +               up_read(&mm->mmap_sem); 
> > 
> > One has to ask, what's wrong with find_vma() ?
> 
> Are you looking for something like this.
> 
>        down_read(&mm->mmap_sem);
> 	for (vma = find_vma(mm, probept); ; vma = vma->vm_next) {
> 	       if (!valid_vma(vma))
> 		       continue;
> 	       u = find_uprobe(vma->vm_file->f_mapping->host,
> 			       probept - vma->vm_start);
> 	       if (u)
> 		       break;
>        }
>        up_read(&mm->mmap_sem); 

How could you ever need to iterate here? There is only a single vma that
covers the probe point, if that doesn't find a uprobe, there isn't any.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 14/20] 14: uprobes: Handing int3 and singlestep exception.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 11:17:11]:

> On Wed, 2011-01-26 at 14:22 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <peterz@infradead.org> [2011-01-25 14:56:19]:
> > 
> > > On Thu, 2010-12-16 at 15:29 +0530, Srikar Dronamraju wrote:
> > > > +               down_read(&mm->mmap_sem);
> > > > +               for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > > > +                       if (!valid_vma(vma))
> > > > +                               continue;
> > > > +                       if (probept < vma->vm_start || probept > vma->vm_end)
> > > > +                               continue;
> > > > +                       u = find_uprobe(vma->vm_file->f_mapping->host,
> > > > +                                       probept - vma->vm_start);
> > > > +                       if (u)
> > > > +                               break;
> > > > +               }
> > > > +               up_read(&mm->mmap_sem); 
> > > 
> > > One has to ask, what's wrong with find_vma() ?
> > 
> > Are you looking for something like this.
> > 
> >        down_read(&mm->mmap_sem);
> > 	for (vma = find_vma(mm, probept); ; vma = vma->vm_next) {
> > 	       if (!valid_vma(vma))
> > 		       continue;
> > 	       u = find_uprobe(vma->vm_file->f_mapping->host,
> > 			       probept - vma->vm_start);
> > 	       if (u)
> > 		       break;
> >        }
> >        up_read(&mm->mmap_sem); 
> 
> How could you ever need to iterate here? There is only a single vma that
> covers the probe point, if that doesn't find a uprobe, there isn't any.

Agree.
So it simplifies to 

        down_read(&mm->mmap_sem);
 	vma = find_vma(mm, probept);
        if (valid_vma(vma)) {
 	       u = find_uprobe(vma->vm_file->f_mapping->host,
 			       probept - vma->vm_start);
        }
        up_read(&mm->mmap_sem); 

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 14/20] 14: uprobes: Handing int3 and singlestep exception.

[Peter Zijlstra]

On Wed, 2011-01-26 at 20:44 +0530, Srikar Dronamraju wrote:
> So it simplifies to 
> 
>         down_read(&mm->mmap_sem);
>         vma = find_vma(mm, probept);
>         if (valid_vma(vma)) {
>                u = find_uprobe(vma->vm_file->f_mapping->host,
>                                probept - vma->vm_start);
>         }
>         up_read(&mm->mmap_sem); 

Almost, the offset within a file is something like:

  (address - vma->vm_start) + (vma->vm_pgoff << PAGE_SHIFT)


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Srikar Dronamraju]

> On Thu, 2010-12-16 at 15:28 +0530, Srikar Dronamraju wrote:
> > +void uprobe_mmap(struct vm_area_struct *vma)
> > +{
> > +       struct list_head tmp_list;
> > +       struct uprobe *uprobe, *u;
> > +       struct mm_struct *mm;
> > +       struct inode *inode;
> > +
> > +       if (!valid_vma(vma))
> > +               return;
> > +
> > +       INIT_LIST_HEAD(&tmp_list);
> > +
> > +       /*
> > +        * The vma was just allocated and this routine gets called
> > +        * while holding write lock for mmap_sem.  Function called
> > +        * in context of a thread that has a reference to mm.
> > +        * Hence no need to take a reference to mm
> > +        */
> > +       mm = vma->vm_mm;
> > +       up_write(&mm->mmap_sem);
> 
> Are you very very sure its a good thing to simply drop the mmap_sem
> here? Also, why?
> 

I actually dont like to release the write_lock and then reacquire it.
write_opcode, which is called thro install_uprobe, i.e to insert the
actual breakpoint instruction takes a read lock on the mmap_sem.
Hence uprobe_mmap gets called in context with write lock on mmap_sem
held, I had to release it before calling install_uprobe.

Another solution, I thought of was to pass a context to write_opcode to
say that map-sem is already acquired by us. But I am not sure that
idea is good enuf. 

> > +       mutex_lock(&uprobes_mutex);
> > +
> > +       inode = vma->vm_file->f_mapping->host;
> 
> Since you just dropped the mmap_sem, what's keeping that vma from going
> away?
> 

How about dropping the mmap_sem after add_to_temp_list and cachng the
vma->vm_start value before calling add_to_temp_list?

Or if you have better ideas, then that would be great.

> > +       add_to_temp_list(vma, inode, &tmp_list);
> > +
> > +       list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
> > +               mm->uprobes_vaddr = vma->vm_start + uprobe->offset;
> > +               install_uprobe(mm, uprobe);
> > +               list_del(&uprobe->pending_list);
> > +       }
> > +       mutex_unlock(&uprobes_mutex);
> > +       down_write(&mm->mmap_sem);
> > +} 
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Peter Zijlstra]

On Wed, 2011-01-26 at 14:33 +0530, Srikar Dronamraju wrote:
> 
> 
> I actually dont like to release the write_lock and then reacquire it.
> write_opcode, which is called thro install_uprobe, i.e to insert the
> actual breakpoint instruction takes a read lock on the mmap_sem.
> Hence uprobe_mmap gets called in context with write lock on mmap_sem
> held, I had to release it before calling install_uprobe. 

Ah, right, so that's going to give you a head-ache ;-)

The moment you release this mmap_sem, the map you're going to install
the probe point in can go away.

The only way to make this work seems to start by holding the mmap_sem
for writing and make a breakpoint install function that assumes its
taken and doesn't try to acquire it again.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 11:20:39]:

> On Wed, 2011-01-26 at 14:33 +0530, Srikar Dronamraju wrote:
> > 
> > 
> > I actually dont like to release the write_lock and then reacquire it.
> > write_opcode, which is called thro install_uprobe, i.e to insert the
> > actual breakpoint instruction takes a read lock on the mmap_sem.
> > Hence uprobe_mmap gets called in context with write lock on mmap_sem
> > held, I had to release it before calling install_uprobe. 
> 
> Ah, right, so that's going to give you a head-ache ;-)
> 
> The moment you release this mmap_sem, the map you're going to install
> the probe point in can go away.
> 
> The only way to make this work seems to start by holding the mmap_sem
> for writing and make a breakpoint install function that assumes its
> taken and doesn't try to acquire it again.
> 


Yes, this can be done.
I would have to do something like this in register_uprobe().

list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list) {
		down_read(&mm->map_sem);
                if (!install_uprobe(mm, uprobe))
                        ret = 0;
		up_read(&mm->map_sem);
                list_del(&mm->uprobes_list);
                mmput(mm);
}

Agree that this is much better than what we have now.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Peter Zijlstra]

On Wed, 2011-01-26 at 20:29 +0530, Srikar Dronamraju wrote:
> list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list) {
>                 down_read(&mm->map_sem);
>                 if (!install_uprobe(mm, uprobe))
>                         ret = 0;
>                 up_read(&mm->map_sem);
>                 list_del(&mm->uprobes_list);
>                 mmput(mm);
> } 

and the tmp_list thing works because new mm's will hit the mmap callback
and you cannot loose mm's due to the refcount, right?


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-26 16:16:49]:

> On Wed, 2011-01-26 at 20:29 +0530, Srikar Dronamraju wrote:
> > list_for_each_entry_safe(mm, tmpmm, &tmp_list, uprobes_list) {
> >                 down_read(&mm->map_sem);
> >                 if (!install_uprobe(mm, uprobe))
> >                         ret = 0;
> >                 up_read(&mm->map_sem);
> >                 list_del(&mm->uprobes_list);
> >                 mmput(mm);
> > } 
> 
> and the tmp_list thing works because new mm's will hit the mmap callback
> and you cannot loose mm's due to the refcount, right?
> 

Right, In other words, the tmp_list has all mm's that have already
running and have this inode mapped as executable text. Those process
that are yet to start or yet to map the inode as executable text
will hit mmap and then we look at inserting the probes thro
uprobes_mmap. 

-- 
Thanks and Regards
Srikar
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-01-25 13:15:41]:

> On Thu, 2010-12-16 at 15:28 +0530, Srikar Dronamraju wrote:
> > +static void search_within_subtree(struct rb_node *n, struct inode *inode,
> > +               struct list_head *tmp_list);
> > +
> > +static void add_to_temp_list(struct vm_area_struct *vma, struct inode *inode,
> > +               struct list_head *tmp_list)
> > +{
> > +       struct uprobe *uprobe;
> > +       struct rb_node *n;
> > +       unsigned long flags;
> > +
> > +       n = uprobes_tree.rb_node;
> > +       spin_lock_irqsave(&treelock, flags);
> > +       while (n) {
> > +               uprobe = rb_entry(n, struct uprobe, rb_node);
> > +               if (match_inode(uprobe, inode, &n)) {
> > +                       list_add(&uprobe->pending_list, tmp_list);
> > +                       search_within_subtree(n, inode, tmp_list);
> > +                       break;
> > +               }
> > +       }
> > +       spin_unlock_irqrestore(&treelock, flags);
> > +}
> > +
> > +static void __search_within_subtree(struct rb_node *p, struct inode *inode,
> > +               struct list_head *tmp_list)
> > +{
> > +       struct uprobe *uprobe;
> > +
> > +       uprobe = rb_entry(p, struct uprobe, rb_node);
> > +       if (match_inode(uprobe, inode, &p)) {
> > +               list_add(&uprobe->pending_list, tmp_list);
> > +               search_within_subtree(p, inode, tmp_list);
> > +       }
> > +
> > +
> > +}
> > +
> > +static void search_within_subtree(struct rb_node *n, struct inode *inode,
> > +               struct list_head *tmp_list)
> > +{
> > +       struct rb_node *p;
> > +
> > +       if (p)
> > +               __search_within_subtree(p, inode, tmp_list);
> > +
> > +       p = n->rb_right;
> > +       if (p)
> > +               __search_within_subtree(p, inode, tmp_list);
> > +} 
> 
> Whee recursion FTW!, you just blew your kernel stack :-)
> 
> Since you sort inode first, offset second, I think you can simply look
> for the first matching inode entry and simply rb_next() until you don't
> match.

Agree that we should get rid of recursion.

I dont think we can simply use rb_next() once we have the first
matching function. There could be a matching inode but a smaller
offset in left that will be missed by rb_next(). (Unless I have
misunderstood rb_next() !!!)

Here are the ways I think we can workaround.
A. change the match_inode() logic to use rb_first/rb_next.
This would make negate the benefit we get from rb_trees because we
have to match every node. Also match_offset might get a little tricky.

B. use the current match_inode but change the search_within_subtree
logic. search_within_subtree() would first find the leftmode node
within the subtree that still has the same inode. Thereafter it will use
rb_next().

Do you have any other ideas?

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] [PATCH 2.6.37-rc5-tip 8/20] 8: uprobes: mmap and fork hooks.

[Peter Zijlstra]

On Wed, 2011-01-26 at 20:39 +0530, Srikar Dronamraju wrote:
> 
> B. use the current match_inode but change the search_within_subtree
> logic. search_within_subtree() would first find the leftmode node
> within the subtree that still has the same inode. Thereafter it will use
> rb_next().
> 
> Do you have any other ideas? 

Look for the right inode but with offset 0, that should get you the
leftmost matching inode, or the entry left of that (depending on how you
build the tree), after that you should be able to iterate all probes of
that inode by using rb_next.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>