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

[Song Liu]

> On Oct 17, 2019, at 7:28 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> 
> 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.
> 

Thanks Oleg!

Attached is v2 of 4/4. 

Song


============================ 8< ===============================