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

[Srikar Dronamraju]

* Song Liu <songliubraving@xxxxxx> [2019-10-17 09:42:22]:

> Attaching uprobe to text section in THP splits the PMD mapped page table
> into PTE mapped entries. On uprobe detach, we would like to regroup PMD
> mapped page table entry to regain performance benefit of THP.
> 
> However, the regroup is broken For perf_event based trace_uprobe. This is
> because perf_event based trace_uprobe calls uprobe_unregister twice on
> close: first in TRACE_REG_PERF_CLOSE, then in TRACE_REG_PERF_UNREGISTER.
> The second call will split the PMD mapped page table entry, which is not
> the desired behavior.
> 
> Fix this by only use FOLL_SPLIT_PMD for uprobe register case.
> 
> Add a WARN() to confirm uprobe unregister never work on huge pages, and
> abort the operation when this WARN() triggers.
> 
> Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> Signed-off-by: Song Liu <songliubraving@xxxxxx>
> ---

Looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>

>  kernel/events/uprobes.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 94d38a39d72e..c74761004ee5 100644
> --- 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,12 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (ret <= 0)
>  		goto put_old;
> 
> +	if (WARN(!is_register && PageCompound(old_page),
> +		 "uprobe unregister should never work on compound page\n")) {
> +		ret = -EINVAL;
> +		goto put_old;
> +	}
> +
>  	/* We are going to replace instruction, update ref_ctr. */
>  	if (!ref_ctr_updated && uprobe->ref_ctr_offset) {
>  		ret = update_ref_ctr(uprobe, mm, is_register ? 1 : -1);
> -- 
> 2.17.1
> 

-- 
Thanks and Regards
Srikar Dronamraju