linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] uprobes: allow for a cache flush after ixol breakpoint creation
@ 2016-12-13 10:40 Marcin Nowakowski
  2016-12-20  7:58 ` [tip:perf/urgent] uprobes: Fix uprobes on MIPS, " tip-bot for Marcin Nowakowski
  0 siblings, 1 reply; 6+ messages in thread
From: Marcin Nowakowski @ 2016-12-13 10:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mips, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Victor Kamensky

Commit 72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol
write' has introduced an arch-specific method to ensure all caches are
flushed appropriately after an instruction is written to an XOL page.

However, when the XOL area is created and the out-of-line breakpoint
instruction is copied, caches are not flushed at all and stale data may
be found in icache.

Replace a simple copy_to_page() with arch_uprobe_copy_ixol() to allow
the arch to ensure all caches are updated accordingly.

This change fixes uprobes on MIPS InterAptiv (tested on Creator Ci40).

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Ingo Molnar <mingo@redhat.com>
CC: Arnaldo Carvalho de Melo <acme@kernel.org>
CC: Alexander Shishkin <alexander.shishkin@linux.intel.com>
CC: Victor Kamensky <victor.kamensky@linaro.org>

---
 kernel/events/uprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f9ec9ad..b5916b4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1194,7 +1194,7 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
 	/* Reserve the 1st slot for get_trampoline_vaddr() */
 	set_bit(0, area->bitmap);
 	atomic_set(&area->slot_count, 1);
-	copy_to_page(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+	arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
 
 	if (!xol_add_vma(mm, area))
 		return area;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:perf/urgent] uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
  2016-12-13 10:40 [PATCH] uprobes: allow for a cache flush after ixol breakpoint creation Marcin Nowakowski
@ 2016-12-20  7:58 ` tip-bot for Marcin Nowakowski
  2016-12-20 13:08   ` Oleg Nesterov
  0 siblings, 1 reply; 6+ messages in thread
From: tip-bot for Marcin Nowakowski @ 2016-12-20  7:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, peterz, alexander.shishkin, acme,
	marcin.nowakowski, acme, mingo, victor.kamensky, jolsa, hpa,
	torvalds, oleg, tglx

Commit-ID:  297e765e390a2ac996000b5f7228cbd84d995174
Gitweb:     http://git.kernel.org/tip/297e765e390a2ac996000b5f7228cbd84d995174
Author:     Marcin Nowakowski <marcin.nowakowski@imgtec.com>
AuthorDate: Tue, 13 Dec 2016 11:40:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 18 Dec 2016 09:42:11 +0100

uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation

Commit:

  72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol write'

... has introduced an arch-specific method to ensure all caches are
flushed appropriately after an instruction is written to an XOL page.

However, when the XOL area is created and the out-of-line breakpoint
instruction is copied, caches are not flushed at all and stale data may
be found in icache.

Replace a simple copy_to_page() with arch_uprobe_copy_ixol() to allow
the arch to ensure all caches are updated accordingly.

This change fixes uprobes on MIPS InterAptiv (tested on Creator Ci40).

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Victor Kamensky <victor.kamensky@linaro.org>
Cc: linux-mips@linux-mips.org
Link: http://lkml.kernel.org/r/1481625657-22850-1-git-send-email-marcin.nowakowski@imgtec.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/uprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f9ec9ad..b5916b4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1194,7 +1194,7 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
 	/* Reserve the 1st slot for get_trampoline_vaddr() */
 	set_bit(0, area->bitmap);
 	atomic_set(&area->slot_count, 1);
-	copy_to_page(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+	arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
 
 	if (!xol_add_vma(mm, area))
 		return area;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:perf/urgent] uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
  2016-12-20  7:58 ` [tip:perf/urgent] uprobes: Fix uprobes on MIPS, " tip-bot for Marcin Nowakowski
@ 2016-12-20 13:08   ` Oleg Nesterov
  2016-12-20 15:21     ` Marcin Nowakowski
  0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2016-12-20 13:08 UTC (permalink / raw)
  To: tip-bot for Marcin Nowakowski
  Cc: linux-tip-commits, linux-kernel, peterz, alexander.shishkin,
	acme, marcin.nowakowski, acme, mingo, victor.kamensky, jolsa,
	hpa, torvalds, tglx

On 12/19, tip-bot for Marcin Nowakowski wrote:
>
> uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
>
> Commit:
>
>   72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol write'
>
> ... has introduced an arch-specific method to ensure all caches are
> flushed appropriately after an instruction is written to an XOL page.

when this page is already mmaped,

> However, when the XOL area is created and the out-of-line breakpoint
> instruction is copied, caches are not flushed at all and stale data may
> be found in icache.

but in this case the page is not mmaped yet, the probed application will
take a page fault if it tries to execute this insn,

> Replace a simple copy_to_page() with arch_uprobe_copy_ixol() to allow
> the arch to ensure all caches are updated accordingly.
>
> This change fixes uprobes on MIPS InterAptiv (tested on Creator Ci40).

OK, I know nothing about MIPS, but could you help me understand this change?

See above. If we really need flush_icache_range() here then perhaps we should
modify install_special_mapping() and/or __do_fault/special_mapping_fault paths
instead?

Oleg.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:perf/urgent] uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
  2016-12-20 13:08   ` Oleg Nesterov
@ 2016-12-20 15:21     ` Marcin Nowakowski
  2016-12-20 17:50       ` Oleg Nesterov
  0 siblings, 1 reply; 6+ messages in thread
From: Marcin Nowakowski @ 2016-12-20 15:21 UTC (permalink / raw)
  To: Oleg Nesterov, tip-bot for Marcin Nowakowski
  Cc: linux-tip-commits, linux-kernel, peterz, alexander.shishkin,
	acme, acme, mingo, victor.kamensky, jolsa, hpa, torvalds, tglx

Hi Oleg,

On 20.12.2016 14:08, Oleg Nesterov wrote:
> On 12/19, tip-bot for Marcin Nowakowski wrote:
>>
>> uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
>>
>> Commit:
>>
>>   72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol write'
>>
>> ... has introduced an arch-specific method to ensure all caches are
>> flushed appropriately after an instruction is written to an XOL page.
>
> when this page is already mmaped,
>
>> However, when the XOL area is created and the out-of-line breakpoint
>> instruction is copied, caches are not flushed at all and stale data may
>> be found in icache.
>
> but in this case the page is not mmaped yet, the probed application will
> take a page fault if it tries to execute this insn,

In case of MIPS (and AFAICT ARM as well, and these are the only 
architectures that implement arch_uprobe_copy_ixol), the cache flushing 
is done through the kernel addresses of that page, so the fact that it 
is not mapped yet is not an issue.

Do I understand correctly that your statement implies that after the 
page fault and mmapping the xol page, the page is guaranteed to be 
updated in the cache? As definitely that is not something that is 
happening at the moment.


>> Replace a simple copy_to_page() with arch_uprobe_copy_ixol() to allow
>> the arch to ensure all caches are updated accordingly.
>>
>> This change fixes uprobes on MIPS InterAptiv (tested on Creator Ci40).
>
> OK, I know nothing about MIPS, but could you help me understand this change?
>
> See above. If we really need flush_icache_range() here then perhaps we should
> modify install_special_mapping() and/or __do_fault/special_mapping_fault paths
> instead?

Are you suggesting that those should be updated to force a cache update?

Marcin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:perf/urgent] uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
  2016-12-20 15:21     ` Marcin Nowakowski
@ 2016-12-20 17:50       ` Oleg Nesterov
  2016-12-21 11:24         ` Ralf Baechle
  0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2016-12-20 17:50 UTC (permalink / raw)
  To: Marcin Nowakowski
  Cc: tip-bot for Marcin Nowakowski, linux-tip-commits, linux-kernel,
	peterz, alexander.shishkin, acme, acme, mingo, victor.kamensky,
	jolsa, hpa, torvalds, tglx

On 12/20, Marcin Nowakowski wrote:
>
> Hi Oleg,
>
> On 20.12.2016 14:08, Oleg Nesterov wrote:
>> On 12/19, tip-bot for Marcin Nowakowski wrote:
>>>
>>> uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
>>>
>>> Commit:
>>>
>>>   72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol write'
>>>
>>> ... has introduced an arch-specific method to ensure all caches are
>>> flushed appropriately after an instruction is written to an XOL page.
>>
>> when this page is already mmaped,
>>
>>> However, when the XOL area is created and the out-of-line breakpoint
>>> instruction is copied, caches are not flushed at all and stale data may
>>> be found in icache.
>>
>> but in this case the page is not mmaped yet, the probed application will
>> take a page fault if it tries to execute this insn,
>
> In case of MIPS (and AFAICT ARM as well, and these are the only  
> architectures that implement arch_uprobe_copy_ixol), the cache flushing  
> is done through the kernel addresses of that page, so the fact that it  
> is not mapped yet is not an issue.

OK, thanks,

> Do I understand correctly that your statement implies that after the  
> page fault and mmapping the xol page, the page is guaranteed to be  
> updated in the cache? As definitely that is not something that is  
> happening at the moment.

Well, I do not know. Let me repeat I don't understand this flush_.*cache
magic.

But. do_read_fault() does

	__do_fault(..., &fault_page, ...);

	alloc_set_pte(fault_page);

and alloc_set_pte() does flush_icache_page(vma, page)... Hmm, which is nop
on MIPS.

>> OK, I know nothing about MIPS, but could you help me understand this change?
>>
>> See above. If we really need flush_icache_range() here then perhaps we should
>> modify install_special_mapping() and/or __do_fault/special_mapping_fault paths
>> instead?
>
> Are you suggesting that those should be updated to force a cache update?

Again, I do not know. But perhaps it makes more sense to actually implement
flush_icache_page() ? Otherwise another user of install_special_mapping()
can hit the same problem?

Oleg.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:perf/urgent] uprobes: Fix uprobes on MIPS, allow for a cache flush after ixol breakpoint creation
  2016-12-20 17:50       ` Oleg Nesterov
@ 2016-12-21 11:24         ` Ralf Baechle
  0 siblings, 0 replies; 6+ messages in thread
From: Ralf Baechle @ 2016-12-21 11:24 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Marcin Nowakowski, tip-bot for Marcin Nowakowski,
	linux-tip-commits, linux-kernel, peterz, alexander.shishkin,
	acme, acme, mingo, victor.kamensky, jolsa, hpa, torvalds, tglx

On Tue, Dec 20, 2016 at 06:50:05PM +0100, Oleg Nesterov wrote:

> >>> Commit:
> >>>
> >>>   72e6ae285a1d ('ARM: 8043/1: uprobes need icache flush after xol write'
> >>>
> >>> ... has introduced an arch-specific method to ensure all caches are
> >>> flushed appropriately after an instruction is written to an XOL page.
> >>
> >> when this page is already mmaped,
> >>
> >>> However, when the XOL area is created and the out-of-line breakpoint
> >>> instruction is copied, caches are not flushed at all and stale data may
> >>> be found in icache.
> >>
> >> but in this case the page is not mmaped yet, the probed application will
> >> take a page fault if it tries to execute this insn,
> >
> > In case of MIPS (and AFAICT ARM as well, and these are the only  
> > architectures that implement arch_uprobe_copy_ixol), the cache flushing  
> > is done through the kernel addresses of that page, so the fact that it  
> > is not mapped yet is not an issue.
> 
> OK, thanks,
> 
> > Do I understand correctly that your statement implies that after the  
> > page fault and mmapping the xol page, the page is guaranteed to be  
> > updated in the cache? As definitely that is not something that is  
> > happening at the moment.
> 
> Well, I do not know. Let me repeat I don't understand this flush_.*cache
> magic.
> 
> But. do_read_fault() does
> 
> 	__do_fault(..., &fault_page, ...);
> 
> 	alloc_set_pte(fault_page);
> 
> and alloc_set_pte() does flush_icache_page(vma, page)... Hmm, which is nop
> on MIPS.
> 
> >> OK, I know nothing about MIPS, but could you help me understand this change?
> >>
> >> See above. If we really need flush_icache_range() here then perhaps we should
> >> modify install_special_mapping() and/or __do_fault/special_mapping_fault paths
> >> instead?
> >
> > Are you suggesting that those should be updated to force a cache update?
> 
> Again, I do not know. But perhaps it makes more sense to actually implement
> flush_icache_page() ? Otherwise another user of install_special_mapping()
> can hit the same problem?

Documentation/cachetlb.txt says about flush_icache_page:

  void flush_icache_page(struct vm_area_struct *vma, struct page *page)
        All the functionality of flush_icache_page can be implemented in
        flush_dcache_page and update_mmu_cache. In the future, the hope
        is to remove this interface completely.

And that's exactly what MIPS already does, thus flush_icache_page() is a
no-op.  The new interfaces flush_dcache_page and update_mmu_cache
generally are much more efficient.

  Ralf

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-12-21 11:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 10:40 [PATCH] uprobes: allow for a cache flush after ixol breakpoint creation Marcin Nowakowski
2016-12-20  7:58 ` [tip:perf/urgent] uprobes: Fix uprobes on MIPS, " tip-bot for Marcin Nowakowski
2016-12-20 13:08   ` Oleg Nesterov
2016-12-20 15:21     ` Marcin Nowakowski
2016-12-20 17:50       ` Oleg Nesterov
2016-12-21 11:24         ` Ralf Baechle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).