Re: [PATCH 6/9] uprobes: flush cache after xol write

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Oleg Nesterov
On 10/16, Rabin Vincent wrote:

>
> 2012/10/15 Oleg Nesterov <[hidden email]>:
> > On 10/14, Rabin Vincent wrote:
> >> Flush the cache so that the instructions written to the XOL area are
> >> visible.
> >>
> >> Signed-off-by: Rabin Vincent <[hidden email]>
> >> ---
> >>  kernel/events/uprobes.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> >> index ca000a9..8c52f93 100644
> >> --- a/kernel/events/uprobes.c
> >> +++ b/kernel/events/uprobes.c
> >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> >>       vaddr = kmap_atomic(area->page);
> >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> >> +     flush_dcache_page(area->page);
> >>       kunmap_atomic(vaddr);
> >
> > I agree... but why under kmap_atomic?
>
> No real reason; I'll move it to after the unmap.

OK. I assume you will send v2.

But this patch looks like a bugfix, flush_dcache_page() is not a nop
on powerpc. So perhaps we should apply this fix right now?

OTOH, I do not understand this stuff, everything is nop on x86. And
when I look into Documentation/cachetlb.txt I am starting to think
that may be this needs flush_icache_user_range instead?

Rabin, Ananth could you clarify this?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Ananth N Mavinakayanahalli-2
On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:

> On 10/16, Rabin Vincent wrote:
> >
> > 2012/10/15 Oleg Nesterov <[hidden email]>:
> > > On 10/14, Rabin Vincent wrote:
> > >> Flush the cache so that the instructions written to the XOL area are
> > >> visible.
> > >>
> > >> Signed-off-by: Rabin Vincent <[hidden email]>
> > >> ---
> > >>  kernel/events/uprobes.c |    1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > >> index ca000a9..8c52f93 100644
> > >> --- a/kernel/events/uprobes.c
> > >> +++ b/kernel/events/uprobes.c
> > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > >>       vaddr = kmap_atomic(area->page);
> > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > >> +     flush_dcache_page(area->page);
> > >>       kunmap_atomic(vaddr);
> > >
> > > I agree... but why under kmap_atomic?
> >
> > No real reason; I'll move it to after the unmap.
>
> OK. I assume you will send v2.
>
> But this patch looks like a bugfix, flush_dcache_page() is not a nop
> on powerpc. So perhaps we should apply this fix right now?

Starting Power5, all Power processers have coherent caches.

> OTOH, I do not understand this stuff, everything is nop on x86. And
> when I look into Documentation/cachetlb.txt I am starting to think
> that may be this needs flush_icache_user_range instead?
>
> Rabin, Ananth could you clarify this?

Yes. We need flush_icache_user_range(). Though for x86 its always been a
nop, one never knows if there is some Power4 or older machine out there
that is still being used. We are fine for Power5 and later.

Ananth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Oleg Nesterov
On 10/26, Ananth N Mavinakayanahalli wrote:

>
> On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > On 10/16, Rabin Vincent wrote:
> > >
> > > >> --- a/kernel/events/uprobes.c
> > > >> +++ b/kernel/events/uprobes.c
> > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > >>       vaddr = kmap_atomic(area->page);
> > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > >> +     flush_dcache_page(area->page);
> > > >>       kunmap_atomic(vaddr);
> > > >
> > > > I agree... but why under kmap_atomic?
> > >
> > > No real reason; I'll move it to after the unmap.
> >
> > OK. I assume you will send v2.
> >
> > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > on powerpc. So perhaps we should apply this fix right now?
>
> Starting Power5, all Power processers have coherent caches.
>
> > OTOH, I do not understand this stuff, everything is nop on x86. And
> > when I look into Documentation/cachetlb.txt I am starting to think
> > that may be this needs flush_icache_user_range instead?
> >
> > Rabin, Ananth could you clarify this?
>
> Yes. We need flush_icache_user_range(). Though for x86 its always been a
> nop, one never knows if there is some Power4 or older machine out there
> that is still being used. We are fine for Power5 and later.

This is bad...

flush_icache_user needs vma. perhaps just to check VM_EXEC...

So let me repeat to be sure I really understand, do you confirm that
_in general_ flush_dcache_page() is not enough?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Ananth N Mavinakayanahalli-2
On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:

> On 10/26, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > > On 10/16, Rabin Vincent wrote:
> > > >
> > > > >> --- a/kernel/events/uprobes.c
> > > > >> +++ b/kernel/events/uprobes.c
> > > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > > >>       vaddr = kmap_atomic(area->page);
> > > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > > >> +     flush_dcache_page(area->page);
> > > > >>       kunmap_atomic(vaddr);
> > > > >
> > > > > I agree... but why under kmap_atomic?
> > > >
> > > > No real reason; I'll move it to after the unmap.
> > >
> > > OK. I assume you will send v2.
> > >
> > > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > > on powerpc. So perhaps we should apply this fix right now?
> >
> > Starting Power5, all Power processers have coherent caches.
> >
> > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > when I look into Documentation/cachetlb.txt I am starting to think
> > > that may be this needs flush_icache_user_range instead?
> > >
> > > Rabin, Ananth could you clarify this?
> >
> > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > nop, one never knows if there is some Power4 or older machine out there
> > that is still being used. We are fine for Power5 and later.
>
> This is bad...
>
> flush_icache_user needs vma. perhaps just to check VM_EXEC...
>
> So let me repeat to be sure I really understand, do you confirm that
> _in general_ flush_dcache_page() is not enough?

flush_dcache_page() on powerpc already checks for
CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Ananth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Oleg Nesterov
On 10/29, Ananth N Mavinakayanahalli wrote:

>
> On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > >
> > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > that may be this needs flush_icache_user_range instead?
> > > >
> > > > Rabin, Ananth could you clarify this?
> > >
> > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > nop, one never knows if there is some Power4 or older machine out there
> > > that is still being used. We are fine for Power5 and later.
> >
> > This is bad...
> >
> > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> >
> > So let me repeat to be sure I really understand, do you confirm that
> > _in general_ flush_dcache_page() is not enough?
>
> flush_dcache_page() on powerpc already checks for
> CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Thanks Ananth.

Still it is not clear to me if flush_dcache_page() would be always right
if we add the new port.

OK. So I assume we need the fix and I am going to apply the patch below.

Ananth, Rabin, will you ack it (including the comment I affed) ?

Oleg.

------------------------------------------------------------------------------
[PATCH] uprobes: flush cache after xol write

From: Rabin Vincent <[hidden email]>

Flush the cache so that the instructions written to the XOL area are
visible.

Signed-off-by: Rabin Vincent <[hidden email]>

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
  vaddr = kmap_atomic(area->page);
  memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
  kunmap_atomic(vaddr);
+ /*
+ * We probably need flush_icache_user_range() but it needs vma.
+ * This should work on supported architectures too.
+ */
+ flush_dcache_page(area->page);
 
  return current->utask->xol_vaddr;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Ananth N Mavinakayanahalli-2
On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:

> On 10/29, Ananth N Mavinakayanahalli wrote:
> >
> > On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > > >
> > > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > > that may be this needs flush_icache_user_range instead?
> > > > >
> > > > > Rabin, Ananth could you clarify this?
> > > >
> > > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > > nop, one never knows if there is some Power4 or older machine out there
> > > > that is still being used. We are fine for Power5 and later.
> > >
> > > This is bad...
> > >
> > > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> > >
> > > So let me repeat to be sure I really understand, do you confirm that
> > > _in general_ flush_dcache_page() is not enough?
> >
> > flush_dcache_page() on powerpc already checks for
> > CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.
>
> Thanks Ananth.
>
> Still it is not clear to me if flush_dcache_page() would be always right
> if we add the new port.
>
> OK. So I assume we need the fix and I am going to apply the patch below.
>
> Ananth, Rabin, will you ack it (including the comment I affed) ?
>
> Oleg.
>
> ------------------------------------------------------------------------------
> [PATCH] uprobes: flush cache after xol write
>
> From: Rabin Vincent <[hidden email]>
>
> Flush the cache so that the instructions written to the XOL area are
> visible.
>
> Signed-off-by: Rabin Vincent <[hidden email]>

Acked-by: Ananth N Mavinakayanahalli <[hidden email]>

>
> --- x/kernel/events/uprobes.c
> +++ x/kernel/events/uprobes.c
> @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
>   vaddr = kmap_atomic(area->page);
>   memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
>   kunmap_atomic(vaddr);
> + /*
> + * We probably need flush_icache_user_range() but it needs vma.
> + * This should work on supported architectures too.
> + */
> + flush_dcache_page(area->page);
>
>   return current->utask->xol_vaddr;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/9] uprobes: flush cache after xol write

Oleg Nesterov
On 11/04, Ananth N Mavinakayanahalli wrote:

>
> On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:
> >
> > [PATCH] uprobes: flush cache after xol write
> >
> > From: Rabin Vincent <[hidden email]>
> >
> > Flush the cache so that the instructions written to the XOL area are
> > visible.
> >
> > Signed-off-by: Rabin Vincent <[hidden email]>
>
> Acked-by: Ananth N Mavinakayanahalli <[hidden email]>

Thanks Ananth.

I assume that Srikar and Rabin agree, applied as 65b6ecc038

> >
> > --- x/kernel/events/uprobes.c
> > +++ x/kernel/events/uprobes.c
> > @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
> >   vaddr = kmap_atomic(area->page);
> >   memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
> >   kunmap_atomic(vaddr);
> > + /*
> > + * We probably need flush_icache_user_range() but it needs vma.
> > + * This should work on supported architectures too.
> > + */
> > + flush_dcache_page(area->page);
> >
> >   return current->utask->xol_vaddr;
> >  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/