[PATCH 0/4] ref_tracker: register debugfs files for each ref_tracker_dir

[PATCH 0/4] ref_tracker: register debugfs files for each ref_tracker_dir

[Jeff Layton]

I had previously sent some patches to add debugfs files for the net
namespace refcount trackers, but Andrew convinced me to make this more
generic and better-integrated into the ref_tracker infrastructure.

This adds a new ref_tracker_dir_debugfs() call that subsystems can call
to finalize the name of their dir and register a debugfs file for it.
The last two patches add these calls for the netns and netdev
ref_trackers.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (4):
      ref_tracker: add a top level debugfs directory for ref_tracker
      ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir
      net: add ref_tracker_dir_debugfs() calls for netns refcount tracking
      net: register debugfs file for net_device refcnt tracker

 include/linux/ref_tracker.h | 13 ++++++
 lib/ref_tracker.c           | 98 ++++++++++++++++++++++++++++++++++++++++++++-
 net/core/dev.c              |  2 +
 net/core/net_namespace.c    | 34 +++++++++++++++-
 4 files changed, 145 insertions(+), 2 deletions(-)
---
base-commit: 695caca9345a160ecd9645abab8e70cfe849e9ff
change-id: 20250413-reftrack-dbgfs-3767b303e2fa

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/4] ref_tracker: add a top level debugfs directory for ref_tracker

[Jeff Layton]

Add a new "ref_tracker" directory in debugfs. Each individual refcount
tracker can register files under there to display info about
currently-held references.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index cf5609b1ca79361763abe5a3a98484a3ee591ff2..c96994134fe1ddfcbf644cc75b36b7e94461ec48 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -12,6 +12,8 @@
 #define REF_TRACKER_STACK_ENTRIES 16
 #define STACK_BUF_SIZE 1024
 
+static struct dentry *ref_tracker_debug_dir;
+
 struct ref_tracker {
 	struct list_head	head;   /* anchor into dir->list or dir->quarantine */
 	bool			dead;
@@ -273,3 +275,19 @@ int ref_tracker_free(struct ref_tracker_dir *dir,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ref_tracker_free);
+
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int __init ref_tracker_debug_init(void)
+{
+	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
+	if (IS_ERR(ref_tracker_debug_dir)) {
+		pr_warn("ref_tracker: unable to create debugfs ref_tracker directory: %pe\n",
+			ref_tracker_debug_dir);
+		ref_tracker_debug_dir = NULL;
+	}
+	return 0;
+}
+late_initcall(ref_tracker_debug_init);
+#endif /* CONFIG_DEBUG_FS */

-- 
2.49.0

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

[Jeff Layton]

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.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/ref_tracker.h | 13 +++++++
 lib/ref_tracker.c           | 84 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 94 insertions(+), 3 deletions(-)

diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h
index 8eac4f3d52547ccbaf9dcd09962ce80d26fbdff8..77a55a32c067216fa02ba349498f53bd289aee0c 100644
--- a/include/linux/ref_tracker.h
+++ b/include/linux/ref_tracker.h
@@ -5,6 +5,7 @@
 #include <linux/types.h>
 #include <linux/spinlock.h>
 #include <linux/stackdepot.h>
+#include <linux/seq_file.h>
 
 struct ref_tracker;
 
@@ -17,6 +18,9 @@ struct ref_tracker_dir {
 	bool			dead;
 	struct list_head	list; /* List of active trackers */
 	struct list_head	quarantine; /* List of dead trackers */
+#ifdef CONFIG_DEBUG_FS
+	struct dentry		*dentry;
+#endif
 	char			name[32];
 #endif
 };
@@ -34,10 +38,15 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
 	dir->dead = false;
 	refcount_set(&dir->untracked, 1);
 	refcount_set(&dir->no_tracker, 1);
+#ifdef CONFIG_DEBUG_FS
+	dir->dentry = NULL;
+#endif
 	strscpy(dir->name, name, sizeof(dir->name));
 	stack_depot_init();
 }
 
+void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name);
+
 void ref_tracker_dir_exit(struct ref_tracker_dir *dir);
 
 void ref_tracker_dir_print_locked(struct ref_tracker_dir *dir,
@@ -62,6 +71,10 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
 {
 }
 
+static inline void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
+{
+}
+
 static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
 {
 }
diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index c96994134fe1ddfcbf644cc75b36b7e94461ec48..10452f66283b081460ef7f4f5640e30487bb1595 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -8,6 +8,7 @@
 #include <linux/slab.h>
 #include <linux/stacktrace.h>
 #include <linux/stackdepot.h>
+#include <linux/seq_file.h>
 
 #define REF_TRACKER_STACK_ENTRIES 16
 #define STACK_BUF_SIZE 1024
@@ -30,6 +31,14 @@ struct ref_tracker_dir_stats {
 	} stacks[];
 };
 
+#ifdef CONFIG_DEBUG_FS
+static void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir);
+#else
+static inline void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir)
+{
+}
+#endif
+
 static struct ref_tracker_dir_stats *
 ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit)
 {
@@ -66,6 +75,7 @@ ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit)
 
 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; \
@@ -163,6 +175,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
 	bool leak = false;
 
 	dir->dead = true;
+	ref_tracker_debugfs_remove(dir);
 	spin_lock_irqsave(&dir->lock, flags);
 	list_for_each_entry_safe(tracker, n, &dir->quarantine, head) {
 		list_del(&tracker->head);
@@ -279,7 +292,72 @@ EXPORT_SYMBOL_GPL(ref_tracker_free);
 #ifdef CONFIG_DEBUG_FS
 #include <linux/debugfs.h>
 
-static int __init ref_tracker_debug_init(void)
+static int ref_tracker_dir_seq_print(struct ref_tracker_dir *dir, struct seq_file *seq)
+{
+	struct ostream os = { .seq = seq };
+	unsigned long flags;
+
+	spin_lock_irqsave(&dir->lock, flags);
+	__ref_tracker_dir_pr_ostream(dir, 16, &os);
+	spin_unlock_irqrestore(&dir->lock, flags);
+
+	return os.used;
+}
+
+static int ref_tracker_debugfs_show(struct seq_file *f, void *v)
+{
+	struct ref_tracker_dir *dir = f->private;
+
+	return ref_tracker_dir_seq_print(dir, f);
+}
+
+static int ref_tracker_debugfs_open(struct inode *inode, struct file *filp)
+{
+	struct ref_tracker_dir *dir = inode->i_private;
+
+	return single_open(filp, ref_tracker_debugfs_show, dir);
+}
+
+static const struct file_operations ref_tracker_debugfs_fops = {
+	.owner		= THIS_MODULE,
+	.open		= ref_tracker_debugfs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+/**
+ * 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.
+ */
+void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
+{
+	strscpy(dir->name, name, sizeof(dir->name));
+
+	if (ref_tracker_debug_dir) {
+		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;
+		}
+	}
+}
+EXPORT_SYMBOL(ref_tracker_dir_debugfs);
+
+static void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir)
+{
+	debugfs_remove(dir->dentry);
+}
+
+static int __init ref_tracker_debugfs_init(void)
 {
 	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
 	if (IS_ERR(ref_tracker_debug_dir)) {
@@ -289,5 +367,5 @@ static int __init ref_tracker_debug_init(void)
 	}
 	return 0;
 }
-late_initcall(ref_tracker_debug_init);
+late_initcall(ref_tracker_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */

-- 
2.49.0

[PATCH 3/4] net: add ref_tracker_dir_debugfs() calls for netns refcount tracking

[Jeff Layton]

After assigning the inode number to the namespace, use it to create a
unique name for each netns refcount tracker and register the debugfs
files for them.

The init_net is registered early in the boot process before the
ref_tracker dir is created, so add a late_initcall() to register its
files.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/core/net_namespace.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 4303f2a4926243e2c0ff0c0387383cd8e0658019..6ffd8aa05c38512e26572d6eada96a36e4aa1ef3 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -761,12 +761,44 @@ struct net *get_net_ns_by_pid(pid_t pid)
 }
 EXPORT_SYMBOL_GPL(get_net_ns_by_pid);
 
+#ifdef CONFIG_NET_NS_REFCNT_TRACKER
+static void net_ns_net_debugfs(struct net *net)
+{
+	char name[32];
+	size_t len;
+
+	len = snprintf(name, sizeof(name), "net-%u-refcnt", net->ns.inum);
+	if (len < sizeof(name))
+		ref_tracker_dir_debugfs(&net->refcnt_tracker, name);
+
+	len = snprintf(name, sizeof(name), "net-%u-notrefcnt", net->ns.inum);
+	if (len < sizeof(name))
+		ref_tracker_dir_debugfs(&net->notrefcnt_tracker, name);
+}
+
+static int __init init_net_debugfs(void)
+{
+	net_ns_net_debugfs(&init_net);
+	return 0;
+}
+late_initcall(init_net_debugfs);
+#else
+static void net_ns_net_debugfs(struct net *net)
+{
+}
+#endif
+
 static __net_init int net_ns_net_init(struct net *net)
 {
+	int ret;
+
 #ifdef CONFIG_NET_NS
 	net->ns.ops = &netns_operations;
 #endif
-	return ns_alloc_inum(&net->ns);
+	ret = ns_alloc_inum(&net->ns);
+	if (!ret)
+		net_ns_net_debugfs(net);
+	return ret;
 }
 
 static __net_exit void net_ns_net_exit(struct net *net)

-- 
2.49.0

[PATCH 4/4] net: register debugfs file for net_device refcnt tracker

[Jeff Layton]

As a nearly-final step in register_netdevice(), finalize the name in the
refcount tracker, and register a debugfs file for it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2f7f5fd9ffec7c0fc219eb6ba57d57a55134186e..db9cac702bb2230ca2bbc2c04ac0a77482c65fc3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10994,6 +10994,8 @@ int register_netdevice(struct net_device *dev)
 	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
 		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
 
+	/* Register debugfs file for the refcount tracker */
+	ref_tracker_dir_debugfs(&dev->refcnt_tracker, dev->name);
 out:
 	return ret;
 

-- 
2.49.0

Re: [PATCH 4/4] net: register debugfs file for net_device refcnt tracker

[Kuniyuki Iwashima]

From: Jeff Layton <jlayton@kernel.org>
Date: Mon, 14 Apr 2025 10:45:49 -0400
> As a nearly-final step in register_netdevice(), finalize the name in the
> refcount tracker, and register a debugfs file for it.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2f7f5fd9ffec7c0fc219eb6ba57d57a55134186e..db9cac702bb2230ca2bbc2c04ac0a77482c65fc3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10994,6 +10994,8 @@ int register_netdevice(struct net_device *dev)
>  	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
>  		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
>  
> +	/* Register debugfs file for the refcount tracker */
> +	ref_tracker_dir_debugfs(&dev->refcnt_tracker, dev->name);

dev->name is not unique across network namespaces, so we should specify
a netns-specific parent dir here.

For example, syzkaller creates a bunch of devices with the same name in
different network namespaces.

Then, we also need to move the file when dev is moved to another netns
in __dev_change_net_namespace().

Re: [PATCH 1/4] ref_tracker: add a top level debugfs directory for ref_tracker

[Andrew Lunn]

> +static int __init ref_tracker_debug_init(void)
> +{
> +	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
> +	if (IS_ERR(ref_tracker_debug_dir)) {

I'm pretty sure GregKH will tell you not to check the return
code. Nothing bad should happen. Yes, it takes a while to get used to
this, but that is the way debugfs is designed.

> +		pr_warn("ref_tracker: unable to create debugfs ref_tracker directory: %pe\n",
> +			ref_tracker_debug_dir);
> +		ref_tracker_debug_dir = NULL;

No need for this. All debugfs_ functions are happy to accept an err
pointer and do a NOP.

	Andrew

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

[Andrew Lunn]

> -static int __init ref_tracker_debug_init(void)

[snip]

> +static int __init ref_tracker_debugfs_init(void)
>  {
>  	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
>  	if (IS_ERR(ref_tracker_debug_dir)) {
> @@ -289,5 +367,5 @@ static int __init ref_tracker_debug_init(void)
>  	}
>  	return 0;
>  }
> -late_initcall(ref_tracker_debug_init);
> +late_initcall(ref_tracker_debugfs_init);

You just added this in the previous patch. Please give it the correct
name from the start.

	Andrew

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

[Andrew Lunn]

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

Re: [PATCH 4/4] net: register debugfs file for net_device refcnt tracker

[Andrew Lunn]

On Mon, Apr 14, 2025 at 03:27:36PM -0700, Kuniyuki Iwashima wrote:
> From: Jeff Layton <jlayton@kernel.org>
> Date: Mon, 14 Apr 2025 10:45:49 -0400
> > As a nearly-final step in register_netdevice(), finalize the name in the
> > refcount tracker, and register a debugfs file for it.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  net/core/dev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 2f7f5fd9ffec7c0fc219eb6ba57d57a55134186e..db9cac702bb2230ca2bbc2c04ac0a77482c65fc3 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10994,6 +10994,8 @@ int register_netdevice(struct net_device *dev)
> >  	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
> >  		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
> >  
> > +	/* Register debugfs file for the refcount tracker */
> > +	ref_tracker_dir_debugfs(&dev->refcnt_tracker, dev->name);
> 
> dev->name is not unique across network namespaces, so we should specify
> a netns-specific parent dir here.
> 
> For example, syzkaller creates a bunch of devices with the same name in
> different network namespaces.
> 
> Then, we also need to move the file when dev is moved to another netns
> in __dev_change_net_namespace().

The address of dev should be unique, and does not change as the netdev
moves between network name spaces. So you could postfix it with the
hashed version of an address, as produced by %pK. This is debugfs, it
does not need to be too friendly.

       Andrew

Re: [PATCH 4/4] net: register debugfs file for net_device refcnt tracker

[Kuniyuki Iwashima]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 15 Apr 2025 01:16:11 +0200
> On Mon, Apr 14, 2025 at 03:27:36PM -0700, Kuniyuki Iwashima wrote:
> > From: Jeff Layton <jlayton@kernel.org>
> > Date: Mon, 14 Apr 2025 10:45:49 -0400
> > > As a nearly-final step in register_netdevice(), finalize the name in the
> > > refcount tracker, and register a debugfs file for it.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  net/core/dev.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 2f7f5fd9ffec7c0fc219eb6ba57d57a55134186e..db9cac702bb2230ca2bbc2c04ac0a77482c65fc3 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -10994,6 +10994,8 @@ int register_netdevice(struct net_device *dev)
> > >  	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
> > >  		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
> > >  
> > > +	/* Register debugfs file for the refcount tracker */
> > > +	ref_tracker_dir_debugfs(&dev->refcnt_tracker, dev->name);
> > 
> > dev->name is not unique across network namespaces, so we should specify
> > a netns-specific parent dir here.
> > 
> > For example, syzkaller creates a bunch of devices with the same name in
> > different network namespaces.
> > 
> > Then, we also need to move the file when dev is moved to another netns
> > in __dev_change_net_namespace().
> 
> The address of dev should be unique, and does not change as the netdev
> moves between network name spaces. So you could postfix it with the
> hashed version of an address, as produced by %pK. This is debugfs, it
> does not need to be too friendly.

Fair enough.

Maybe %p should be used ?  I just saw this series.
https://lore.kernel.org/netdev/20250414-restricted-pointers-net-v1-0-12af0ce46cdd@linutronix.de/

Also, it would be nice to update netdev_wait_allrefs_any()
so that we can query the debugfs easily once we find the
possible refcnt leak.

---8<---
diff --git a/net/core/dev.c b/net/core/dev.c
index 93f982de1da0..d129e44ef9f9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11277,8 +11279,8 @@ static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
 		if (time_after(jiffies, warning_time +
 			       READ_ONCE(netdev_unregister_timeout_secs) * HZ)) {
 			list_for_each_entry(dev, list, todo_list) {
-				pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
-					 dev->name, netdev_refcnt_read(dev));
+				pr_emerg("unregister_netdevice: waiting for %s (%p) to become free. Usage count = %d\n",
+					 dev->name, dev, netdev_refcnt_read(dev));
 				ref_tracker_dir_print(&dev->refcnt_tracker, 10);
 			}
 
---8<---

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

[Jeff Layton]

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@kernel.org>

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

[Jeff Layton]

On Tue, 2025-04-15 at 06:23 -0400, Jeff Layton wrote:
> 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.
> 
> > 

I took a crack at this and it's trickier than it looks. We have to pass
a variadic function pointer (which is fine), but that means that they
can't use handy macros like pr_err. We have to call functions that can
take a va_list (which is also fine).

The part I'm having trouble with is incorporating the pr_fmt(). I've
attached the patch I have on top of the current series. It doesn't
compile for me. If I remove the pr_fmt() calls and just pass in the
format string, it does compile.

What am I doing wrong here?

-------------------------8<---------------------------------

[PATCH] ref_tracker: have callers pass output function to pr_ostream

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index 4cc49cc21f5b..e131fdc51838 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -77,21 +77,41 @@ struct ostream {
 	char *buf;
 	struct seq_file *seq;
 	int size, used;
+	void (*func)(struct ostream *stream, char *fmt, ...);
 };
 
+#define ref_tracker_log(fmt, args) vprintk_emit(0, LOGLEVEL_ERR, NULL,
pr_fmt(fmt), args)
+
+static void pr_ostream_log(struct ostream *stream, char *fmt, ...)
+{
+	va_list args;
+
+	ref_tracker_log(fmt, args);
+}
+
+#define ref_tracker_vsnprintf(buf, size, fmt, args) vsnprintf(buf,
size, pr_fmt(fmt), args)
+
+static void pr_ostream_buf(struct ostream *stream, char *fmt, ...)
+{
+	int ret, len = stream->size - stream->used;
+	va_list args;
+
+	ret = ref_tracker_vsnprintf(stream->buf + stream->used, len,
fmt, args);
+	stream->used += min(ret, len);
+}
+
+static void pr_ostream_seq(struct ostream *stream, char *fmt, ...)
+{
+	va_list args;
+
+	seq_vprintf(stream->seq, fmt, args);
+}
+
 #define pr_ostream(stream, fmt, args...) \
 ({ \
 	struct ostream *_s = (stream); \
 \
-	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; \
-		ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt),
##args); \
-		_s->used += min(ret, len); \
-	} \
+	_s->func(_s, fmt, ##args); \
 })
 
 static void
@@ -138,7 +158,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir
*dir,
 void ref_tracker_dir_print_locked(struct ref_tracker_dir *dir,
 				  unsigned int display_limit)
 {
-	struct ostream os = {};
+	struct ostream os = { .func = pr_ostream_log };
 
 	__ref_tracker_dir_pr_ostream(dir, display_limit, &os);
 }
@@ -157,7 +177,7 @@ EXPORT_SYMBOL(ref_tracker_dir_print);
 
 int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf,
size_t size)
 {
-	struct ostream os = { .buf = buf, .size = size };
+	struct ostream os = { .buf = buf, .size = size, .func =
pr_ostream_buf };
 	unsigned long flags;
 
 	spin_lock_irqsave(&dir->lock, flags);
@@ -294,7 +314,7 @@ EXPORT_SYMBOL_GPL(ref_tracker_free);
 
 static int ref_tracker_dir_seq_print(struct ref_tracker_dir *dir,
struct seq_file *seq)
 {
-	struct ostream os = { .seq = seq };
+	struct ostream os = { .seq = seq, .func = pr_ostream_seq };
 	unsigned long flags;
 
 	spin_lock_irqsave(&dir->lock, flags);
-- 
2.49.0

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

[Jeff Layton]

On Tue, 2025-04-15 at 08:54 -0400, Jeff Layton wrote:
> On Tue, 2025-04-15 at 06:23 -0400, Jeff Layton wrote:
> > 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.
> > 
> > > 
> 
> I took a crack at this and it's trickier than it looks. We have to pass
> a variadic function pointer (which is fine), but that means that they
> can't use handy macros like pr_err. We have to call functions that can
> take a va_list (which is also fine).
> 
> The part I'm having trouble with is incorporating the pr_fmt(). I've
> attached the patch I have on top of the current series. It doesn't
> compile for me. If I remove the pr_fmt() calls and just pass in the
> format string, it does compile.
> 
> What am I doing wrong here?
> 
> -------------------------8<---------------------------------
> 
> [PATCH] ref_tracker: have callers pass output function to pr_ostream
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  lib/ref_tracker.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
> index 4cc49cc21f5b..e131fdc51838 100644
> --- a/lib/ref_tracker.c
> +++ b/lib/ref_tracker.c
> @@ -77,21 +77,41 @@ struct ostream {
>  	char *buf;
>  	struct seq_file *seq;
>  	int size, used;
> +	void (*func)(struct ostream *stream, char *fmt, ...);
>  };
>  
> +#define ref_tracker_log(fmt, args) vprintk_emit(0, LOGLEVEL_ERR, NULL,
> pr_fmt(fmt), args)
> +
> +static void pr_ostream_log(struct ostream *stream, char *fmt, ...)
> +{
> +	va_list args;
> +
> +	ref_tracker_log(fmt, args);
> +}
> +

Oh, duh, I figured it out -- the problem is that the compiler can't
implicitly concatenate a string literal and a variable (of course).

I think we can just pass a prefix string in struct ostream that we can
add into the pr_ostream calls. I'll plan to send an updated set after a
bit more testing.
-- 
Jeff Layton <jlayton@kernel.org>