> On Oct 16, 2019, at 5:10 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > On 10/16, Song Liu wrote: >> >> --- a/kernel/events/uprobes.c >> +++ b/kernel/events/uprobes.c >> @@ -474,14 +474,17 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, >> struct vm_area_struct *vma; >> int ret, is_register, ref_ctr_updated = 0; >> bool orig_page_huge = false; >> + unsigned int gup_flags = FOLL_FORCE; >> >> is_register = is_swbp_insn(&opcode); >> uprobe = container_of(auprobe, struct uprobe, arch); >> >> retry: >> + if (is_register) >> + gup_flags |= FOLL_SPLIT_PMD; >> /* Read the page with vaddr into memory */ >> - ret = get_user_pages_remote(NULL, mm, vaddr, 1, >> - FOLL_FORCE | FOLL_SPLIT_PMD, &old_page, &vma, NULL); >> + ret = get_user_pages_remote(NULL, mm, vaddr, 1, gup_flags, >> + &old_page, &vma, NULL); >> if (ret <= 0) >> return ret; >> >> @@ -489,6 +492,9 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, >> if (ret <= 0) >> goto put_old; >> >> + WARN(!is_register && PageCompound(old_page), >> + "uprobe unregister should never work on compound page\n"); > > But this can happen with the change above. You can't know if *vaddr was > previously changed by install_breakpoint() or not. > If not, verify_opcode() should likely save us, but we can't rely on it. > Say, someone can write "int3" into vm_file at uprobe->offset. I think this won't really happen. With is_register == false, we already know opcode is not "int3", so current call must be from set_orig_insn(). Therefore, old_page must be installed by uprobe, and cannot be compound. The other way is not guaranteed. With is_register == true, it is still possible current call is from set_orig_insn(). However, we do not rely on this path. Does this make sense? Or did I miss anything? > > And I am not sure it is safe to continue in this case, I'd suggest to > return -EWHATEVER to avoid the possible crash. I think we can return -ESOMETHING here to be safe. However, if the analysis above makes sense, it is not necessary. Thanks, Song
