Re: [PATCH 4/4] uprobe: only do FOLL_SPLIT_PMD for uprobe register

From: Oleg Nesterov <oleg@redhat.com>
To: Song Liu <songliubraving@fb.com>
Cc: open list <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"matthew.wilcox@oracle.com" <matthew.wilcox@oracle.com>,
	Kernel Team <Kernel-team@fb.com>,
	"william.kucharski@oracle.com" <william.kucharski@oracle.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Subject: Re: [PATCH 4/4] uprobe: only do FOLL_SPLIT_PMD for uprobe register
Date: Thu, 17 Oct 2019 16:28:24 +0200	[thread overview]
Message-ID: <20191017142824.GA453@redhat.com> (raw)
In-Reply-To: <A1DBC6EA-CBAB-45FE-919D-6D77D29DDE1D@fb.com>

On 10/17, Song Liu wrote:
>
>
> > On Oct 17, 2019, at 1:47 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 10/16, Song Liu wrote:
> >>
> >>> On Oct 16, 2019, at 5:10 AM, Oleg Nesterov <oleg@redhat.com> 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.