On 10/17, Song Liu wrote: > > > > On Oct 17, 2019, at 1:47 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > > > On 10/16, Song Liu wrote: > >> > >>> On Oct 16, 2019, at 5:10 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > >>> > >>>> @@ -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. > > > > Quite contrary. > > > > When is_register == true we know that a) the caller is install_breakpoint() > > and b) the original insn is NOT int3 unless this page was alreadt COW'ed by > > userspace, say, by gdb. > > > > If is_register == false we only know that the caller is remove_breakpoint(). > > We can't know if this page was COW'ed by uprobes or userspace, we can not > > know if the insn we are going to replace is int3 or not, thus we can not > > assume that verify_opcode() will fail and save us. > > So the case we worry about is: > old_page is COW by user space, no, in this case the page shouldn't be huge, > target insn is int3, and it is a huge page; > then uprobe calls remove_breakpoint(); Yes, > Yeah, I guess this could happen. Yes, > For the fix, I guess return -Esomething in such case should be sufficient? this is what I tried to suggest from the very beginning. Oleg.
