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