Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Aug 01, 2024 at 02:00:18PM +0200, Oleg Nesterov wrote:
> On 08/01, Jiri Olsa wrote:
> >
> > > @@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
> > >  {
> > >         mutex_lock(&testmod_uprobe_mutex);
> > >
> > > -       if (uprobe.offset) {
> > > -               uprobe_unregister(d_real_inode(uprobe.path.dentry),
> > > -                                 uprobe.offset, &uprobe.consumer);
> > > +       if (uprobe.uprobe) {
> > > +               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
> > >                 uprobe.offset = 0;
> > > +               uprobe.uprobe = NULL;
> >
> > ugh, I think we leak &uprobe.path.. I can send follow up fix if needed
> 
> Yeah, with or without this change. And with this change we do not need uprobe.offset.
> 
> Please see the patch below, this is what I've added to 5/5.
> 
> Do you see any problems?

looks good, thanks!

> 
> Note the additional path_put() in testmod_unregister_uprobe(). Does it need
> a separate patch or can it come with 5/5 ?

I think it'd be better to have it separately, the test is already
released.. so people might want to backport just the fix

thanks,
jirka

> 
> Oleg.
> 
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
>  
>  struct testmod_uprobe {
>  	struct path path;
> -	loff_t offset;
> +	struct uprobe *uprobe;
>  	struct uprobe_consumer consumer;
>  };
>  
> @@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset)
>  {
>  	int err = -EBUSY;
>  
> -	if (uprobe.offset)
> +	if (uprobe.uprobe)
>  		return -EBUSY;
>  
>  	mutex_lock(&testmod_uprobe_mutex);
>  
> -	if (uprobe.offset)
> +	if (uprobe.uprobe)
>  		goto out;
>  
>  	err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
>  	if (err)
>  		goto out;
>  
> -	err = uprobe_register(d_real_inode(uprobe.path.dentry),
> -			      offset, 0, &uprobe.consumer);
> -	if (err)
> +	uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
> +					offset, 0, &uprobe.consumer);
> +	if (IS_ERR(uprobe.uprobe)) {
> +		err = PTR_ERR(uprobe.uprobe);
>  		path_put(&uprobe.path);
> -	else
> -		uprobe.offset = offset;
> -
> +		uprobe.uprobe = NULL;
> +	}
>  out:
>  	mutex_unlock(&testmod_uprobe_mutex);
>  	return err;
> @@ -474,10 +474,10 @@ static void testmod_unregister_uprobe(void)
>  {
>  	mutex_lock(&testmod_uprobe_mutex);
>  
> -	if (uprobe.offset) {
> -		uprobe_unregister(d_real_inode(uprobe.path.dentry),
> -				  uprobe.offset, &uprobe.consumer);
> -		uprobe.offset = 0;
> +	if (uprobe.uprobe) {
> +		uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
> +		path_put(&uprobe.path);
> +		uprobe.uprobe = NULL;
>  	}
>  
>  	mutex_unlock(&testmod_uprobe_mutex);
>