Re: [PATCHv4 02/13] uprobes/x86: Remove struct uprobe_trampoline object

[Oleg Nesterov <oleg@redhat.com>]

On 05/26, Jiri Olsa wrote:
>
> Removing struct uprobe_trampoline object and it's tracking code,
> because it's not needed. We can do same thing directly on top of
> struct vm_area_struct objects.
>
> This makes the code simpler and allows easy propagation of the
> trampoline vma object into child process in following change.
>
> Note the original code called destroy_uprobe_trampoline if the
> optimiation failed, but it only freed the struct uprobe_trampoline
> object, not the vma. The new vma leak is fixed in following change.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

---------------------------------------------------------------------
Although I can't convince myself I fully understand this code with or
without this patch ;)

A couple of questions below...

> -static struct uprobe_trampoline *create_uprobe_trampoline(unsigned long vaddr)
> +static struct vm_area_struct *get_uprobe_trampoline(struct mm_struct *mm, unsigned long vaddr)
>  {
> -	struct pt_regs *regs = task_pt_regs(current);
> -	struct mm_struct *mm = current->mm;
> -	struct uprobe_trampoline *tramp;
> +	VMA_ITERATOR(vmi, mm, 0);
>  	struct vm_area_struct *vma;
>
> -	if (!user_64bit_mode(regs))
> -		return NULL;
> +	if (vaddr > TASK_SIZE || vaddr < PAGE_SIZE)
> +		return ERR_PTR(-EINVAL);

Do we really need this check? It looks a bit confusing to me...
vaddr is bp_vaddr from handle_swbp(), it should be valid?

> +
> +	for_each_vma(vmi, vma) {
> +		if (!vma_is_special_mapping(vma, &tramp_mapping))
> +			continue;
> +		if (is_reachable_by_call(vma->vm_start, vaddr))
> +			return vma;
> +	}

Perhaps we can later optimize this code a bit? I mean something like

	start_reachable = ...;
	end_reachable = ...;

	VMA_ITERATOR(vmi, mm, start_reachable);

	for_each_vma(vmi, vma) {
		if (!vma_is_special_mapping(...))
			continue;
		if (vma->vm_start > end_reachable)
			break;
		return vma;
	}

>  static int __arch_uprobe_optimize(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  				  unsigned long vaddr)
>  {
> -	struct uprobe_trampoline *tramp;
> -	struct vm_area_struct *vma;
> -	bool new = false;
> -	int err = 0;
> +	struct pt_regs *regs = task_pt_regs(current);
> +	struct vm_area_struct *vma, *tramp;
>
> +	if (!user_64bit_mode(regs))
> +		return -EINVAL;
>  	vma = find_vma(mm, vaddr);
>  	if (!vma)
>  		return -EINVAL;

I guess find_vma() can't fail, the caller arch_uprobe_optimize() has called
copy_from_vaddr() under mmap_write_lock()... Nevermind.

Oleg.