linux-kernel - Re: [PATCH v2 5/5] uprobes: make uprobe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqt8UPBC7zAWDMHD@krava>
Date: Thu, 1 Aug 2024 14:15:12 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Jiri Olsa <olsajiri@...il.com>,
	Andrii Nakryiko <andrii.nakryiko@...il.com>, andrii@...nel.org,
	mhiramat@...nel.org, peterz@...radead.org, rostedt@...dmis.org,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct
 uprobe *

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);
>