Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()
From: Oleg Nesterov <oleg@redhat.com>
Date: 2025-02-22 12:40:40
Also in:
bpf, linux-mm, linux-perf-users, lkml
On 02/22, Tong Tiangen wrote:
I'm going to add a new patch to moving the "verify_opcode()" check down , IIUC that "!PageAnon(old_page)" below also needs to be moved together,
No, no. I forgot everything, but please don't do this. IIUC This is optimization for the case when the probed file has int3 at this offset. We should not skip update_ref_ctr() in this case, just we can avoid __replace_page().
and as David said this can be triggered by user space, so delete the use of "WARN", as follows:
Hmm... I think that David meant the new WARN_ON() added by you in V1? Please don't remove the old WARN(PageCompound(old_page) check. If userspace can trigger this warning we need to to fix this code and add FOLL_SPLIT_PMD unconditionally (and likely do something else). I take my words back ;) Don't do the additional cleanups, just add the is_zero_page(old_page) check right after get_user_page_vma_remote() and update the subject/changelog as David suggests. This function needs more cleanups anyway. Say, the usage of orig_page_huge _looks_ obviously wrong even if (afaics) nothing bad can happen. It should be reinitialized after "goto retry" or it should be checked before the "orig_page = find_get_page()" code. The usage of gup_flags looks confusing too. Lets do this later. Oleg.
quoted hunk ↗ jump to hunk
@@ -502,20 +502,16 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe,struct mm_struct *mm, if (IS_ERR(old_page)) return PTR_ERR(old_page); - ret = verify_opcode(old_page, vaddr, &opcode); - if (ret <= 0) + ret = -EINVAL; + if (is_zero_page(old_page)) goto put_old; - if (is_zero_page(old_page)) { - ret = -EINVAL; + if (!is_register && (PageCompound(old_page) || !PageAnon(old_page))) goto put_old; - } - if (WARN(!is_register && PageCompound(old_page), - "uprobe unregister should never work on compound page\n")) { - ret = -EINVAL; + ret = verify_opcode(old_page, vaddr, &opcode); + if (ret <= 0) goto put_old; - } /* We are going to replace instruction, update ref_ctr. */ if (!ref_ctr_updated && uprobe->ref_ctr_offset) {@@ -526,10 +522,6 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe,struct mm_struct *mm, ref_ctr_updated = 1; } - ret = 0; - if (!is_register && !PageAnon(old_page)) - goto put_old; - ret = anon_vma_prepare(vma); Thanks.quoted
quoted
..