Re: [RFC PATCH v2 02/11] ptwrite uprobes: Add minimal low level support for x86

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] x86/uprobes: Uninitialized variable first_phase_done causes invalid fallback
- [High] x86/uprobes: Missing __GFP_ACCOUNT bypasses memcg and GFP_KERNEL deadlocks
- [Medium] x86/uprobes: GFP_HIGHUSER allocation pins unmigratable pages in movable zones
--

commit 692de39e6024ec893235089efa2253699ad076bf
Author: Andi Kleen <ak@kernel.org>

ptwrite uprobes: Add minimal low level support for x86

Add more data structures and the x86 machinery to generate the PTWRITE
instructions for a ptwrite uprobe. The probe executes PTWRITEs and then
jumps back to the original code. In this variant only patching
5 byte nops is supported.

> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 65a2de82ecd29..7fcdc4bf5197a 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
[ ... ]
> @@ -719,6 +723,134 @@ static struct vm_area_struct *get_uprobe_trampoline(struct mm_struct *mm, unsign
[ ... ]
> +int uprobe_ptwrite_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> +{
> +	struct uprobes_state *old_state = &oldmm->uprobes_state;
> +	struct uprobes_state *new_state = &newmm->uprobes_state;
> +	struct uprobe_ptwrite_page *ptw, *new;
> +	struct vm_area_struct *vma;
> +	struct hlist_node *n;
> +
> +	mmap_assert_write_locked(oldmm);
> +	mmap_assert_write_locked(newmm);
> +	hlist_for_each_entry(ptw, &old_state->head_ptwrite, node) {
> +		void *src, *dst;
> +
> +		/*
> +		 * Not using __GFP_ACCOUNT here because it triggered
> +		 * a deadlock.
> +		 */
> +		new = kzalloc_obj(*new, GFP_KERNEL);
> +		if (!new)
> +			goto fail;
> +		new->page = alloc_page(GFP_KERNEL | __GFP_ZERO);

[Severity: High]
Does stripping __GFP_ACCOUNT here allow containers to bypass memory cgroup
limits? By repeatedly forking processes with active ptwrite uprobes, a local
user could potentially consume unbounded kernel memory without being charged.

Also, since this is called under the mmap_write_lock of both the old and new
mm_structs, is it safe to use GFP_KERNEL allocations? GFP_KERNEL can enter
direct reclaim, which might attempt to reacquire the same or other locks via
page writeback or swap-out, leading to deadlocks.

[ ... ]
> @@ -1102,6 +1287,475 @@ static int copy_from_vaddr(struct mm_struct *mm, unsigned long vaddr, void *dst,
[ ... ]
> +static struct uprobe_ptwrite_page *
> +create_uprobe_ptwrite_page(struct mm_struct *mm, unsigned long vaddr)
> +{
> +	struct uprobe_ptwrite_page *ptw;
> +	struct vm_area_struct *vma;
> +	unsigned long area;
> +
> +	area = find_ptwrite_page_area(mm, vaddr);
> +	if (IS_ERR_VALUE(area))
> +		return NULL;
> +
> +	mmap_assert_write_locked(mm);
> +
> +	ptw = kzalloc_obj(*ptw, GFP_KERNEL);
> +	if (!ptw)
> +		return NULL;
> +
> +	ptw->page = alloc_page(GFP_HIGHUSER | __GFP_ZERO);

[Severity: Medium]
Will this allocation pin an unmigratable page in a movable zone?

The GFP_HIGHUSER mask includes the __GFP_MOVABLE flag, but the returned page
is a raw page mapped via _install_special_mapping(). Since it lacks movable_ops
or LRU linkage, it is fundamentally unmigratable. This breaks the page
allocator's mobility contract and can cause permanent memory fragmentation.

[ ... ]
> +static int ptwrite_text_poke(struct arch_uprobe *auprobe,
> +			     struct vm_area_struct *vma, unsigned long vaddr,
> +			     unsigned long stub_addr)
> +{
> +	u8 jmp5[5] = { 0xe9, 0, 0, 0, 0 };
> +	bool first_phase_done;
> +	s32 rel;
> +	int err;
> +
> +	if (!ptwrite_rel32(vaddr + 5, stub_addr, &rel))
> +		return -ERANGE;
> +	memcpy(jmp5 + 1, &rel, 4);
> +
> +	err = text_poke_5byte(auprobe, vma, vaddr, jmp5,
> +			      auprobe->ptwrite.orig[0], false, true, true,
> +			      false, &first_phase_done);
> +	if (err && first_phase_done) {

[Severity: High]
Can this evaluate uninitialized memory on the error path?

If text_poke_5byte() fails during its first uprobe_write() call (when setting
the INT3 breakpoint), it returns the error immediately without initializing the
first_phase_done out-parameter:

arch/x86/kernel/uprobes.c:text_poke_5byte() {
    ...
	if (!skip_int3) {
		err = uprobe_write(auprobe, vma, vaddr, &int3, 1, verify_insn,
				   is_register, false, &ctx);
		if (err)
			return err;
	}
	if (first_phase_done)
		*first_phase_done = true;
    ...
}

If the uninitialized first_phase_done variable happens to be non-zero (true),
ptwrite_text_poke() will attempt a rollback by calling text_poke_5byte() again
to remove an INT3 breakpoint that was never successfully installed, which could
further fail or corrupt the instruction stream.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917230127.924985-2-ak@kernel.org?part=2