Re: [PATCH v2] tracing/uprobe: Add missing PID filter for uretprobe

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Aug 27, 2024 at 12:40:52PM +0200, Oleg Nesterov wrote:
> On 08/27, Jiri Olsa wrote:
> >
> > On Mon, Aug 26, 2024 at 01:57:52PM +0200, Oleg Nesterov wrote:
> > >
> > > So perhaps we need
> > >
> > > 	-	if (link->task && current->mm != link->task->mm)
> > > 	+	if (link->task && !same_thread_group(current, link->task))
> > >
> > > in uprobe_prog_run() to make "filter by *process*" true, but this won't
> > > fix the problem with link->task->mm == NULL in uprobe_multi_link_filter().
> >
> > would the same_thread_group(current, link->task) work in such case?
> > (zombie leader with other alive threads)
> 
> Why not? task_struct->signal is stable, it can't be changed.
> 
> But again, uprobe_multi_link_filter() won't work if the leader,
> uprobe->link->task, exits or it has already exited.
> 
> Perhaps something like the additional change below...
> 
> Oleg.
> 
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3322,13 +3322,28 @@ static int uprobe_prog_run(struct bpf_uprobe *uprobe,
>  	return err;
>  }
>  
> +
>  static bool
>  uprobe_multi_link_filter(struct uprobe_consumer *con, struct mm_struct *mm)
>  {
>  	struct bpf_uprobe *uprobe;
> +	struct task_struct *task, *t;
> +	bool ret = false;
>  
>  	uprobe = container_of(con, struct bpf_uprobe, consumer);
> -	return uprobe->link->task->mm == mm;
> +	task = uprobe->link->task;
> +
> +	rcu_read_lock();
> +	for_each_thread(task, t) {
> +		struct mm_struct *mm = READ_ONCE(t->mm);
> +		if (mm) {
> +			ret = t->mm == mm;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();

that seems expensive if there's many threads

could we check the leader first and only if it's gone fallback to this?

thanks,
jirka