Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Oleg Nesterov]

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.

>
>
> @@ -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.
> >
> >>
> >>
> >>.
> >
> >.
>