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 <[email protected]> > Signed-off-by: Jiri Olsa <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]> --------------------------------------------------------------------- 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.
