linux-kernel - 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: <b697c51c0788e4e462e45a5cc78d3b5c2d01d496.camel@kernel.org>
Date: Tue, 15 Apr 2025 06:23:10 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
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 Tue, 2025-04-15 at 01:08 +0200, Andrew Lunn wrote:
> 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?
> 

Sure. I actually had this part in a separate patch earlier, but I don't
usually like adding functions with no callers and this patch was pretty
small. I can break it up though.

> >  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.
> 

Maybe? It doesn't seem worth it for this, but I'll take a look.

> > +/**
> > + * 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.
> 

Ok.

> > + */
> > +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?
> 

I think it's safe. The only thing that currently uses ->name is
pr_ostream(), AFAICT.

> > +	if (ref_tracker_debug_dir) {
> 
> Not needed
> 

If we call debugfs_create_file() and pass in NULL as the parent, it
will try to create the entry at the root of debugfs. We can get rid of
this though if we initialize ref_tracker_debug_dir to an ERR_PTR value.
I'll see about doing that.

> > +		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.
> 

Only if ref_tracker_debug_dir is initialized to an ERR_PTR.
-- 
Jeff Layton <jlayton@...nel.org>