linux-kernel - Re: [PATCH 13/13] xfs: convert xfs_sysfs attrs to use ->seq

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]

Date:   Mon, 13 Sep 2021 08:27:50 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Christoph Hellwig <hch@....de>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Jens Axboe <axboe@...nel.dk>, Tejun Heo <tj@...nel.org>,
        linux-block@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/13] xfs: convert xfs_sysfs attrs to use ->seq_show

On Mon, Sep 13, 2021 at 07:41:21AM +0200, Christoph Hellwig wrote:
> Trivial conversion to the seq_file based sysfs attributes.
> 
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
>  fs/xfs/xfs_stats.c | 24 +++++-------
>  fs/xfs/xfs_stats.h |  2 +-
>  fs/xfs/xfs_sysfs.c | 96 +++++++++++++++++++++++-----------------------
>  3 files changed, 58 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 20e0534a772c9..71e7a84ba0403 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -16,10 +16,9 @@ static int counter_val(struct xfsstats __percpu *stats, int idx)
>  	return val;
>  }
>  
> -int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
> +void xfs_stats_format(struct xfsstats __percpu *stats, struct seq_file *sf)
>  {
>  	int		i, j;
> -	int		len = 0;
>  	uint64_t	xs_xstrat_bytes = 0;
>  	uint64_t	xs_write_bytes = 0;
>  	uint64_t	xs_read_bytes = 0;
> @@ -58,13 +57,12 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  	/* Loop over all stats groups */
>  
>  	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
> -		len += scnprintf(buf + len, PATH_MAX - len, "%s",
> -				xstats[i].desc);
> +		seq_printf(sf, "%s", xstats[i].desc);
> +
>  		/* inner loop does each group */
>  		for (; j < xstats[i].endpoint; j++)
> -			len += scnprintf(buf + len, PATH_MAX - len, " %u",
> -					counter_val(stats, j));
> -		len += scnprintf(buf + len, PATH_MAX - len, "\n");
> +			seq_printf(sf, " %u", counter_val(stats, j));
> +		seq_printf(sf, "\n");
>  	}
>  	/* extra precision counters */
>  	for_each_possible_cpu(i) {
> @@ -74,18 +72,14 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		defer_relog += per_cpu_ptr(stats, i)->s.defer_relog;
>  	}
>  
> -	len += scnprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
> +	seq_printf(sf, "xpc %Lu %Lu %Lu\n",
>  			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
> -	len += scnprintf(buf + len, PATH_MAX-len, "defer_relog %llu\n",
> -			defer_relog);
> -	len += scnprintf(buf + len, PATH_MAX-len, "debug %u\n",
> +	seq_printf(sf, "defer_relog %llu\n", defer_relog);
>  #if defined(DEBUG)
> -		1);
> +	seq_printf(sf, "debug 1\n");
>  #else
> -		0);
> +	seq_printf(sf, "debug 0\n");
>  #endif
> -
> -	return len;
>  }

That is a sysfs file?  What happened to the "one value per file" rule
here?

Ugh.

Anyway, I like the idea, but as you can see here, it could lead to even
more abuse of sysfs files.  We are just now getting people to use
sysfs_emit() and that is showing us where people have been abusing the
api in bad ways.

Is there any way that sysfs can keep the existing show functionality and
just do the seq_printf() for the buffer returned by the attribute file
inside of the sysfs core?  That would allow us to keep all of the
existing attribute file functions as-is, and still get rid of the sysfs
core usage here?

thanks,

greg k-h