Re: [PATCH v2 04/12] uprobes: revamp uprobe refcounting and lifetime management

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 4 Jul 2024 10:45:24 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jul 04, 2024 at 10:03:48AM +0200, Peter Zijlstra wrote:
> 
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index c98e3b3386ba..4aafb4485be7 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -1112,7 +1112,8 @@ static void __probe_event_disable(struct trace_probe *tp)
> >  		if (!tu->inode)
> >  			continue;
> >  
> > -		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
> > +		uprobe_unregister(tu->inode, tu->offset, &tu->consumer,
> > +				  list_is_last(trace_probe_probe_list(tp), &tu->tp.list) ? 0 : URF_NO_SYNC);
> >  		tu->inode = NULL;
> >  	}
> >  }
> 
> 
> Hmm, that continue clause might ruin things. Still easy enough to add
> uprobe_unregister_sync() and simpy always pass URF_NO_SYNC.
> 
> I really don't see why we should make this more complicated than it
> needs to be.
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 354cab634341..681741a51df3 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -115,7 +115,9 @@ extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern int uprobe_register_refctr(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
>  extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
> -extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> +#define URF_NO_SYNC	0x01
> +extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags);
> +extern void uprobe_unregister_sync(void);
>  extern int uprobe_mmap(struct vm_area_struct *vma);
>  extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
>  extern void uprobe_start_dup_mmap(void);
> @@ -165,7 +167,7 @@ uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, boo
>  	return -ENOSYS;
>  }
>  static inline void
> -uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
> +uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags)

nit: IMHO, I would like to see uprobe_unregister_nosync() variant instead of
adding flags.

Thank you,

>  {
>  }
>  static inline int uprobe_mmap(struct vm_area_struct *vma)
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 0b7574a54093..d09f7b942076 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1145,7 +1145,7 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>   * @offset: offset from the start of the file.
>   * @uc: identify which probe if multiple probes are colocated.
>   */
> -void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
> +void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags)
>  {
>  	scoped_guard (srcu, &uprobe_srcu) {
>  		struct uprobe *uprobe = find_uprobe(inode, offset);
> @@ -1157,10 +1157,17 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
>  		mutex_unlock(&uprobe->register_mutex);
>  	}
>  
> -	synchronize_srcu(&uprobe_srcu); // XXX amortize / batch
> +	if (!(flags & URF_NO_SYNC))
> +		synchronize_srcu(&uprobe_srcu);
>  }
>  EXPORT_SYMBOL_GPL(uprobe_unregister);
>  
> +void uprobe_unregister_sync(void)
> +{
> +	synchronize_srcu(&uprobe_srcu);
> +}
> +EXPORT_SYMBOL_GPL(uprobe_unregister_sync);
> +
>  /*
>   * __uprobe_register - register a probe
>   * @inode: the file in which the probe has to be placed.
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d1daeab1bbc1..1f6adabbb1e7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3181,9 +3181,10 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
>  	u32 i;
>  
>  	for (i = 0; i < cnt; i++) {
> -		uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
> -				  &uprobes[i].consumer);
> +		uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset, URF_NO_SYNC);
>  	}
> +	if (cnt > 0)
> +		uprobe_unregister_sync();
>  }
>  
>  static void bpf_uprobe_multi_link_release(struct bpf_link *link)
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..6b64470a1c5c 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1104,6 +1104,7 @@ static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter)
>  static void __probe_event_disable(struct trace_probe *tp)
>  {
>  	struct trace_uprobe *tu;
> +	bool sync = false;
>  
>  	tu = container_of(tp, struct trace_uprobe, tp);
>  	WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter));
> @@ -1112,9 +1113,12 @@ static void __probe_event_disable(struct trace_probe *tp)
>  		if (!tu->inode)
>  			continue;
>  
> -		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
> +		uprobe_unregister(tu->inode, tu->offset, &tu->consumer, URF_NO_SYNC);
> +		sync = true;
>  		tu->inode = NULL;
>  	}
> +	if (sync)
> +		uprobe_unregister_sync();
>  }
>  
>  static int probe_event_enable(struct trace_event_call *call,


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>