mbox series

[v3,0/3] kernel/events/uprobes: uprobe_write_opcode() rewrite

Message ID 20250321113713.204682-1-david@redhat.com (mailing list archive)
Headers show
Series kernel/events/uprobes: uprobe_write_opcode() rewrite | expand

Message

David Hildenbrand March 21, 2025, 11:37 a.m. UTC 
Based on mm/unstable.

Currently, uprobe_write_opcode() implements COW-breaking manually, which is
really far from ideal. Further, there is interest in supporting uprobes on
hugetlb pages [1], and leaving at least the COW-breaking to the core will
make this much easier.

Also, I think the current code doesn't really handle some things
properly (see patch #3) when replacing/zapping pages.

Let's rewrite it, to leave COW-breaking to the fault handler, and handle
registration/unregistration by temporarily unmapping the anonymous page,
modifying it, and mapping it again. We still have to implement
zapping of anonymous pages ourselves, unfortunately.

We could look into not performing the temporary unmapping if we can
perform the write atomically, which would likely also make adding hugetlb
support a lot easier. But, limited (e.g., only PMD/PUD) hugetlb support
could be added on top of this with some tweaking.

Note that we now won't have to allocate another anonymous folio when
unregistering (which will be beneficial for hugetlb as well), we can simply
modify the already-mapped one from the registration (if any). When
registering a uprobe, we'll first trigger a ptrace-like write fault to
break COW, to then modify the already-mapped page.

Briefly sanity tested with perf probes and with the bpf uprobes
selftest.

v2 -> v3:
* Fix missing folio_put()

v1 -> v2:
* "kernel/events/uprobes: uprobe_write_opcode() rewrite"
 -> hold GUP reference longer so we can see if the page is still
    mapped when performing the folio_walk
 -> Move anon-folio check
 -> Reshuffle / cleanup some related things

RFC -> v1:
* Use folio_walk and simplify the logic

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: Tong Tiangen <tongtiangen@huawei.com>

[1] https://lkml.kernel.org/r/ZiK50qob9yl5e0Xz@bender.morinfr.org

David Hildenbrand (3):
  kernel/events/uprobes: pass VMA instead of MM to remove_breakpoint()
  kernel/events/uprobes: pass VMA to set_swbp(), set_orig_insn() and
    uprobe_write_opcode()
  kernel/events/uprobes: uprobe_write_opcode() rewrite

 arch/arm/probes/uprobes/core.c |   4 +-
 include/linux/uprobes.h        |   6 +-
 kernel/events/uprobes.c        | 357 +++++++++++++++++----------------
 3 files changed, 187 insertions(+), 180 deletions(-)


base-commit: a150906197a12c7b0f3f5efd844443bf98453efa

Comments

Oleg Nesterov March 21, 2025, 1:34 p.m. UTC | #1 
On 03/21, David Hildenbrand wrote:
>
> Based on mm/unstable.

which has another fix from David in __replace_page() plus the fix from
Tong in uprobe_write_opcode().

So, Andrew, could you take this series as well? Peter, will you agree?

> Currently, uprobe_write_opcode() implements COW-breaking manually, which is
> really far from ideal.

It was never ideal, but today it is simply horrible ;)
Thanks again for your work.

Oleg.
Peter Zijlstra March 25, 2025, 10:53 a.m. UTC | #2 
On Fri, Mar 21, 2025 at 02:34:01PM +0100, Oleg Nesterov wrote:
> On 03/21, David Hildenbrand wrote:
> >
> > Based on mm/unstable.
> 
> which has another fix from David in __replace_page() plus the fix from
> Tong in uprobe_write_opcode().
> 
> So, Andrew, could you take this series as well? Peter, will you agree?

Yeah,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>