linux-kernel - Re: [PATCH 2/2] rv: Convert to use _

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: <cb44571efe8157f126e519b7cf1932ad7feda7fb.camel@redhat.com>
Date: Mon, 17 Nov 2025 08:27:11 +0100
From: Gabriele Monaco <gmonaco@...hat.com>
To: Nam Cao <namcao@...utronix.de>, Steven Rostedt <rostedt@...dmis.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers
	 <mathieu.desnoyers@...icios.com>, linux-trace-kernel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] rv: Convert to use __free

On Sun, 2025-11-16 at 15:35 +0000, Nam Cao wrote:
> Convert to use __free to tidy up the code.
> 
> Signed-off-by: Nam Cao <namcao@...utronix.de>

Both patches look very neat, thanks!

I have a few comments on this one:

> ---
>  kernel/trace/rv/rv.c          | 65 +++++++++++++++--------------------
>  kernel/trace/rv/rv.h          |  6 ++--
>  kernel/trace/rv/rv_reactors.c | 32 ++++++++---------
>  3 files changed, 46 insertions(+), 57 deletions(-)
> 
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index b1059a3cf4fa..e83dc9f0c7bb 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -420,35 +420,28 @@ static const struct file_operations interface_desc_fops
> = {
>  static int create_monitor_dir(struct rv_monitor *mon, struct rv_monitor
> *parent)
>  {
> ... 
> +	mon->root_d = dir;
> +	retain_and_null_ptr(dir);

I think you could just save a line by doing:

+	mon->root_d = no_free_ptr(dir);

>  	return 0;
> -
> -out_remove_root:
> -	rv_remove(mon->root_d);
> -	return retval;
>  }
>  
>  /*
> @@ -827,39 +820,37 @@ int __init rv_init_interface(void)
>  {
>  	struct dentry *tmp;
>  	int retval;
> +	struct dentry *root_dir __free(rv_remove) = rv_create_dir("rv",
> NULL);
>  
> -	rv_root.root_dir = rv_create_dir("rv", NULL);
> -	if (!rv_root.root_dir)
> -		goto out_err;
> +	if (!root_dir)
> +		return 1;
>  
> -	rv_root.monitors_dir = rv_create_dir("monitors", rv_root.root_dir);
> +	rv_root.monitors_dir = rv_create_dir("monitors", root_dir);
>  	if (!rv_root.monitors_dir)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("available_monitors", RV_MODE_READ,
> rv_root.root_dir, NULL,
> +	tmp = rv_create_file("available_monitors", RV_MODE_READ, root_dir,
> NULL,
>  			     &available_monitors_ops);
>  	if (!tmp)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE,
> rv_root.root_dir, NULL,
> +	tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, root_dir,
> NULL,
>  			     &enabled_monitors_ops);
>  	if (!tmp)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("monitoring_on", RV_MODE_WRITE,
> rv_root.root_dir, NULL,
> +	tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, root_dir, NULL,
>  			     &monitoring_on_fops);
>  	if (!tmp)
> -		goto out_err;
> -	retval = init_rv_reactors(rv_root.root_dir);
> +		return 1;
> +	retval = init_rv_reactors(root_dir);
>  	if (retval)
> -		goto out_err;
> +		return 1;
>  
>  	turn_monitoring_on();
>  
> -	return 0;
> +	rv_root.root_dir = root_dir;
> +	retain_and_null_ptr(root_dir);

Same as above:

+	mon->root_d = no_free_ptr(root_dir);

>  
> -out_err:
> -	rv_remove(rv_root.root_dir);
> -	printk(KERN_ERR "RV: Error while creating the RV interface\n");

By removing this, you are also not logging in case of failure, right?
I'm still liking to be rid of goto's, but what about moving this error reporting
to the caller tracer_init_tracefs() ?

> -	return 1;
> +	return 0;
>  }
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index 1485a70c1bf4..8661d1b6ede4 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -17,6 +17,8 @@ struct rv_interface {
>  #define rv_create_file			tracefs_create_file
>  #define rv_remove			tracefs_remove
>  
> +DEFINE_FREE(rv_remove, struct dentry *, rv_remove(_T));

It's kind of idiomatic to do things like `if (_T) free(_T)` in those.
Although free (like tracefs_remove) checks for null, it's probably safer to keep
it that way here as well (as the entire system, including the return_ptr magic
relies on it):

+DEFINE_FREE(rv_remove, struct dentry *, if (_T) rv_remove(_T));

> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index 37d1d37a27a3..ce7d22cbde57 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -438,30 +439,25 @@ static struct rv_reactor rv_nop = {
>  
>  int init_rv_reactors(struct dentry *root_dir)
>  {
> -	struct dentry *available, *reacting;
>  	int retval;
>  
> -	available = rv_create_file("available_reactors", RV_MODE_READ,
> root_dir, NULL,
> -				   &available_reactors_ops);
> -	if (!available)
> -		goto out_err;
> +	struct dentry *available __free(rv_remove) =
> +		rv_create_file("available_reactors", RV_MODE_READ, root_dir,
> +				NULL, &available_reactors_ops);
>  
> -	reacting = rv_create_file("reacting_on", RV_MODE_WRITE, root_dir,
> NULL, &reacting_on_fops);
> -	if (!reacting)
> -		goto rm_available;
> +	struct dentry *reacting =
> +		rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL,
> &reacting_on_fops);

Nothing is removing "reacting_on" in case of successive failure, is it?
Am I missing anything or couldn't we just set both variables to __free() ?

Thanks,
Gabriele

> +
> +	if (!reacting || !available)
> +		return -ENOMEM;
>  
>  	retval = __rv_register_reactor(&rv_nop);
>  	if (retval)
> -		goto rm_reacting;
> +		return retval;
>  
>  	turn_reacting_on();
>  
> +	retain_and_null_ptr(available);
> +	retain_and_null_ptr(reacting);
>  	return 0;
> -
> -rm_reacting:
> -	rv_remove(reacting);
> -rm_available:
> -	rv_remove(available);
> -out_err:
> -	return -ENOMEM;
>  }