netdev - Re: [PATCH 2/4] ref_tracker: add ability to register a file in debugfs for a ref_tracker

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: <a86aab21-c539-48f5-bad1-25db9b8f3ced@lunn.ch>
Date: Tue, 15 Apr 2025 01:08:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jeff Layton <jlayton@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Qasim Ijaz <qasdev00@...il.com>,
	Nathan Chancellor <nathan@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 2/4] ref_tracker: add ability to register a file in
 debugfs for a ref_tracker_dir

On Mon, Apr 14, 2025 at 10:45:47AM -0400, Jeff Layton wrote:
> Currently, there is no convenient way to see the info that the
> ref_tracking infrastructure collects. Add a new function that other
> subsystems can optionally call to update the name field in the
> ref_tracker_dir and register a corresponding seq_file for it in the
> top-level ref_tracker directory.
> 
> Also, alter the pr_ostream infrastructure to allow the caller to specify
> a seq_file to which the output should go instead of printing to an
> arbitrary buffer or the kernel's ring buffer.

When i see an Also, or And, or a list in a commit message, i always
think, should this be multiple patches?

>  struct ostream {
>  	char *buf;
> +	struct seq_file *seq;
>  	int size, used;
>  };
>  
> @@ -73,7 +83,9 @@ struct ostream {
>  ({ \
>  	struct ostream *_s = (stream); \
>  \
> -	if (!_s->buf) { \
> +	if (_s->seq) { \
> +		seq_printf(_s->seq, fmt, ##args); \
> +	} else if (!_s->buf) { \
>  		pr_err(fmt, ##args); \
>  	} else { \
>  		int ret, len = _s->size - _s->used; \

The pr_ostream() macro is getting pretty convoluted. It currently
supports two user cases:

struct ostream os = {}; which means just use pr_err().

And os.buf points to an allocated buffer and the output should be
dumped there.

You are about to add a third.

Is it about time this got split up into three helper functions, and
you pass one to __ref_tracker_dir_pr_ostream()? Your choice.

> +/**
> + * ref_tracker_dir_debugfs - create debugfs file for ref_tracker_dir
> + * @dir: ref_tracker_dir to finalize
> + * @name: updated name of the ref_tracker_dir
> + *
> + * In some cases, the name given to a ref_tracker_dir is based on incomplete information,
> + * and may not be unique. Call this to finalize the name of @dir, and create a debugfs
> + * file for it.

Maybe extend the documentation with a comment that is name is not
unique within debugfs directory, a warning will be emitted but it is
not fatal to the tracker.

> + */
> +void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
> +{
> +	strscpy(dir->name, name, sizeof(dir->name));

I don't know about this. Should we really overwrite the name passed
earlier? Would it be better to treat the name here only as the debugfs
filename?

> +	if (ref_tracker_debug_dir) {

Not needed

> +		dir->dentry = debugfs_create_file(dir->name, S_IFREG | 0400,
> +						  ref_tracker_debug_dir, dir,
> +						  &ref_tracker_debugfs_fops);
> +		if (IS_ERR(dir->dentry)) {
> +			pr_warn("ref_tracker: unable to create debugfs file for %s: %pe\n",
> +				dir->name, dir->dentry);
> +			dir->dentry = NULL;

this last statement should also be unneeded.

	Andrew