| Message ID | 20140408152735.GA30076@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 08, 2014 at 05:27:35PM +0200, Oleg Nesterov wrote: > Sorry, I din't read the whole thread yet... > > On 04/08, Russell King - ARM Linux wrote: > > On Tue, Apr 08, 2014 at 09:05:49AM -0400, David Long wrote: > > > Unfortunately copy_to_user_page() also needs a pointer to a vma struct > > > so, while it presumably provides the model to follow, it can't simply be > > > dropped in. > > > > Well, isn't this code doing the same thing as ptrace? It seems to want > > to modify a page in userspace of another process to change instructions > > that are going to be executed. That's what ptrace does, and ptrace > > already copes with all the issues there. > > Yes, but it does get_user_pages(&vma) and thus it knows vma. > > > Given that we've already solved that problem, wouldn't it be a good idea > > if the tracing code would stop trying to reinvent broken solutions to > > problems we have already solved? > > But uprobes can't do this. Of course, I am not saying this is impossible, > but it would be nice to avoid mmap_sem/find_vma/etc. > > Almost nobody (iirc only sparc?) actually uses this "vma" arguments. And > at least the supported architectures do not (at least this is what I think > after the quick grep). > > Perhaps we can rolerate the hack below? This has no effect at fixing the reported problem though: #define flush_icache_user_range(vma,page,addr,len) \ flush_dcache_page(page) so it results in no change. The bigger question is... what is this function supposed to do? It's not been documented in Documentation/cachetlb.txt, and nothing in the kernel refers to this function - it is completely unused. I think let's start out by killing this function - the semantics of this function have been lost, so it's not clear what it was supposed to do in its original form. Even going back to the start of git history, it looks like it was never used outside arch code.
On Tue, Apr 08, 2014 at 04:41:17PM +0100, Russell King - ARM Linux wrote: > I think let's start out by killing this function - the semantics of > this function have been lost, so it's not clear what it was supposed > to do in its original form. Even going back to the start of git > history, it looks like it was never used outside arch code. I floated a patch to remove flush_icache_user_range() to the architecture maintainers, giving the background to how this came about. I received the following reply from David Miller: > ptrace() accesses (via __access_remote_vm()) already use an existing > helper function for these sorts of situations, in the form of > copy_{to,from}_user_page(). I would suggest that uprobes uses that > as well. I think this is a very valid point, and echo's my point.
On Wed, Apr 09, 2014 at 05:18:26PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 08, 2014 at 04:41:17PM +0100, Russell King - ARM Linux wrote: > > I think let's start out by killing this function - the semantics of > > this function have been lost, so it's not clear what it was supposed > > to do in its original form. Even going back to the start of git > > history, it looks like it was never used outside arch code. > > I floated a patch to remove flush_icache_user_range() to the architecture > maintainers, giving the background to how this came about. I received the > following reply from David Miller: > > > ptrace() accesses (via __access_remote_vm()) already use an existing > > helper function for these sorts of situations, in the form of > > copy_{to,from}_user_page(). I would suggest that uprobes uses that > > as well. > > I think this is a very valid point, and echo's my point. More comments from David about using copy_{to,from}_user_page(): > They should use it at first, then if there is a problem we can identify > exactly what can be optimized or needs to be, and expand the interface > as needed. > > Let's not optimize first. Please look into using this solution, thanks.
On 04/09, Russell King - ARM Linux wrote: > > I floated a patch to remove flush_icache_user_range() to the architecture > maintainers, giving the background to how this came about. I received the > following reply from David Miller: > > > ptrace() accesses (via __access_remote_vm()) already use an existing > > helper function for these sorts of situations, in the form of > > copy_{to,from}_user_page(). I would suggest that uprobes uses that > > as well. > > I think this is a very valid point, and echo's my point. Well. So far I disagree, but let me reply tomorrow. Sorry, right now I can't even read this thread. Oleg.
--- x/kernel/events/uprobes.c +++ x/kernel/events/uprobes.c @@ -1295,10 +1295,10 @@ static unsigned long xol_get_insn_slot(s copy_to_page(area->page, xol_vaddr, &uprobe->arch.ixol, sizeof(uprobe->arch.ixol)); /* - * We probably need flush_icache_user_range() but it needs vma. - * This should work on supported architectures too. + * The architectures we currently support do not really use vma. */ - flush_dcache_page(area->page); + flush_icache_user_range(NULL /* vma */, area->page, + xol_vaddr, sizeof(uprobe->arch.ixol)); return xol_vaddr; }