From: Peter Zijlstra on
On Thu, 2010-05-06 at 23:31 +0530, Srikar Dronamraju wrote:
> - Addressed comments from Oleg, including removal of interrupt context
> handlers, reverting background page replacement in favour of
> access_process_vm().


> +static int write_opcode(struct task_struct *tsk, unsigned long vaddr,
> + user_bkpt_opcode_t opcode)
> +{
> + int ret;
> +
> + if (!tsk)
> + return -EINVAL;
> +
> + ret = access_process_vm(tsk, vaddr, &opcode, user_bkpt_opcode_sz, 1);
> + return (ret == user_bkpt_opcode_sz ? 0 : -EFAULT);
> +}

Why!

That's not not the atomic sequence outlined.

--
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
* Peter Zijlstra <peterz(a)infradead.org> [2010-05-11 22:59:45]:

> On Thu, 2010-05-06 at 23:31 +0530, Srikar Dronamraju wrote:
> > - Addressed comments from Oleg, including removal of interrupt context
> > handlers, reverting background page replacement in favour of
> > access_process_vm().
>
>
> > +static int write_opcode(struct task_struct *tsk, unsigned long vaddr,
> > + user_bkpt_opcode_t opcode)
> > +{
> > + int ret;
> > +
> > + if (!tsk)
> > + return -EINVAL;
> > +
> > + ret = access_process_vm(tsk, vaddr, &opcode, user_bkpt_opcode_sz, 1);
> > + return (ret == user_bkpt_opcode_sz ? 0 : -EFAULT);
> > +}
>
> Why!
>
> That's not not the atomic sequence outlined.
>


Yes, we had moved away from access_process_vm to background page
replacement in Version 1 and Version 2.

One of the reasons being Mathieu suggesting to Jim in LFCS that
for almost all architectures insertion of a breakpoint instruction on a
user page is an atomic operation, as far as the CPU is concerned.

Can you and other VM experts tell me if access_process_vm isnt going to
be atomic with respect to inserting/deleting a breakpoint instruction?

Oleg had few questions which I didnt have answers. (Most of
which you have already answered yesterday). One thing that's still
missing is

[ snipping from Oleg's mail: ]
-----
But suppose that the application does mprotect(PROT_WRITE)
after register_uprobe() installs the bp, now unregister_uprobe/etc
can't restore the original insn?
---

Also I tried a write_opcode that uses background page replacement which
addressed some of Oleg's comments. The pseudo-code is here:
write_opcode()
{
down_read(mmap_sem);

get_user_pages(tsk, mm, vaddr, .. &old_page, &vma);

anon_vma_prepare(vma);

new_page=alloc_page_vma(.., vma, vaddr);

copy_user_page(new_page, old_page);

kmap_atomic(new_page,...);

memcpy(vaddr,..);

kunmap_atomic(..);

lock_page(new_page);

old_pte = get_pte(mm,vaddr);

replace_page(vma, new_page, old_page, old_pte);

unlock_page(new_page);

put_page(new_page);

put_page(old_page);

up_read(mmap);
}


Will this work?

The Other VM quieries that I had were:

Is there any thing else needed for the parent process to pass on the anon_vma to
the child process. (I inserted a breakpoint in the parent and tried
removing the breakpoint in the child.
However page_address_in_vma() (called by replace_page() returned
EFAULT because "vma->anon_vma != page_anon_vma(page)"

Do we need to take care of mem_cgroups?
Do we need to update mm counters?


--
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: Frederic Weisbecker on
On Thu, May 06, 2010 at 11:31:39PM +0530, Srikar Dronamraju wrote:
> intro.patch
>
> From: Srikar Dronamraju <srikar(a)linux.vnet.ibm.com>
>
> Uprobes Patches



I can't find the trace event patch inside this set.
May be you missed it somehow? (or I did).

Thanks.

--
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: Ananth N Mavinakayanahalli on
On Wed, May 12, 2010 at 02:13:05PM +0200, Peter Zijlstra wrote:
> On Wed, 2010-05-12 at 15:55 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <peterz(a)infradead.org> [2010-05-11 22:59:45]:
> >
> > > On Thu, 2010-05-06 at 23:31 +0530, Srikar Dronamraju wrote:
> > > > - Addressed comments from Oleg, including removal of interrupt context
> > > > handlers, reverting background page replacement in favour of
> > > > access_process_vm().
> > >
> > >
> > > > +static int write_opcode(struct task_struct *tsk, unsigned long vaddr,
> > > > + user_bkpt_opcode_t opcode)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + if (!tsk)
> > > > + return -EINVAL;
> > > > +
> > > > + ret = access_process_vm(tsk, vaddr, &opcode, user_bkpt_opcode_sz, 1);
> > > > + return (ret == user_bkpt_opcode_sz ? 0 : -EFAULT);
> > > > +}
> > >
> > > Why!
> > >
> > > That's not not the atomic sequence outlined.
> > >
> >
> >
> > Yes, we had moved away from access_process_vm to background page
> > replacement in Version 1 and Version 2.
> >
> > One of the reasons being Mathieu suggesting to Jim in LFCS that
> > for almost all architectures insertion of a breakpoint instruction on a
> > user page is an atomic operation, as far as the CPU is concerned.
>
> That is true, however when restoring the old instruction you do need to
> take care, so your usage from set_orig_insn() is dubious.
>
> > Can you and other VM experts tell me if access_process_vm isnt going to
> > be atomic with respect to inserting/deleting a breakpoint instruction?
>
> Well, clearly not, since it simply does a gup(.force=1), if whatever
> page is there is writable it will simply poke at it.
>
> Now writing the INT3 is special, but restoring the old insn is not and
> might confuse another CPU that is currently trying to execute code near
> there.

Yes, this helps for breakpoint insertion, but...

The question is whether only INT3 special or single-byte changes are
also guaranteed to be atomic. In http://lkml.org/lkml/2010/1/27/275
Peter Anvin states 'specific case of a more generic rule'.

For restoring the old instruction, we technically need to put back just
one byte, irrespective of the actual length of the underlying
instruction. Now, as long as we have the housekeeping code to handle the
possibility of a thread hitting the said breakpoint when its being
removed, is it safe to assume atomicity for replacing one byte of
possibly a longer instruction?

Ananth
--
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
On Wed, 2010-05-12 at 18:57 +0530, Ananth N Mavinakayanahalli wrote:
> Now, as long as we have the housekeeping code to handle the
> possibility of a thread hitting the said breakpoint when its being
> removed, is it safe to assume atomicity for replacing one byte of
> possibly a longer instruction?

Dunno I'm not a hardware guy, but the issue is so simple to side-step
I'm not sure why you're arguing for relying on these special semantics.



--
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/
 |  Next  |  Last
Pages: 1 2 3
Prev: BUG: amd64-agp (2.6.34-rc7)
Next: [RFC] PyTimechart