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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZqESi1-NgX6XSN-d@pluto>
Date: Wed, 24 Jul 2024 15:41:15 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Luke Parkin <luke.parkin@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	arm-scmi@...r.kernel.org, sudeep.holla@....com,
	cristian.marussi@....com
Subject: Re: [PATCH v3 5/5] firmware: arm_scmi: Reset statistics

On Mon, Jul 15, 2024 at 02:37:51PM +0100, Luke Parkin wrote:
> Create write function to reset individual statistics on write
> Create reset_all stats debugfs file to reset all statistics
> 
> Signed-off-by: Luke Parkin <luke.parkin@....com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 104 +++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9378e2d8af4f..6a90311f764d 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2865,6 +2865,47 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static int read_atomic(void *atomic, u64 *val)
> +{
> +	atomic_t *atm = (atomic_t *)atomic;
> +
> +	*val = atomic_read(atm);
> +	return 0;
> +}
> +
> +static int reset_single(void *atomic, u64 val)
> +{
> +	atomic_t *atm = (atomic_t *)atomic;
> +
> +	atomic_set(atm, 0);
> +	return 0;
> +}
> +

So, you rightly built a fops_reset_on_write to handle any write to a
single stats field and zero teh counter...BUT...for the sake of
simplicity, we could relax a bit the requirement and just think about
handling generically the writes...because if you look at the definition
of debugfs_create_atomic_t:

https://elixir.bootlin.com/linux/v6.10/source/fs/debugfs/file.c#L867

you will see that if you declare the mode as RW 0600, the debugfs core
code will automagically provide you with RW debugfs atomic fops...so
that you wont need all of this and you can still keep using
debugfs_create_atomic_t....the only limitation is that the user will be
able to reset the counter to any value, NOT only to zero...BUT being a
debug/test feats seems to me acceptable.


> +static void reset_all_stats(struct scmi_info *info)
> +{
> +	for (int i = 0; i < LAST; i++)
> +		atomic_set(&info->dbg_stats[i], 0);
> +}

You can drop this function and just nest the for loop inside the
function below...

> +
> +static ssize_t reset_all_on_write(struct file *filp,
> +				  const char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct scmi_info *info = filp->private_data;
> +
> +	reset_all_stats(info);
> +	return count;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_reset_on_write, read_atomic, reset_single, "%llu\n");
> +

..and drop this as said above to keep using debugfs_create_atomic_t

> +static const struct file_operations fops_reset_stats = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.llseek = no_llseek,
> +	.write = reset_all_on_write,
> +};
> +

This is good instead for a quick general reset...

>  static void scmi_debugfs_stats_setup(struct scmi_info *info,
>  				     struct dentry *trans)
>  {
> @@ -2872,32 +2913,43 @@ static void scmi_debugfs_stats_setup(struct scmi_info *info,
>  
>  	stats = debugfs_create_dir("stats", trans);
>  
> -	debugfs_create_atomic_t("sent_ok", 0400, stats,

using 0600 here made the trick...

> -				&info->dbg_stats[SENT_OK]);

Thanks,
Cristian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ