linux-kernel - Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info

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] [day] [month] [year] [list]

Message-ID: <cc695a916d9abd79fa1cd9725c767c7e294a3b39.camel@kernel.org>
Date: Mon, 14 Apr 2025 08:48:26 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: "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>, Andrew Morton
 <akpm@...ux-foundation.org>, 	netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] net: add debugfs files for showing netns
 refcount tracking info

On Mon, 2025-04-14 at 14:46 +0200, Andrew Lunn wrote:
> > > > We'll need some input from the i915 folks then.
> > > 
> > > That is why i think it would be better to add a warning, give the i915
> > > folks a heads up, and leave them to fix it how they want. We want the
> > > warning anyway to cover new refcount instances being added.
> > > 
> > 
> > There will definitely be a pr_warn() if creation fails. I was hoping to
> > send a suggested change alongside the patchset, but I may have to just
> > leave it up to them.
> > 
> > I threw together a draft patchset that auto-registers a debugfs file
> > for every call to ref_tracker_dir_init(). The problem I've hit now
> > though is that at least in the networking cases, the kernel calls
> > ref_tracker_dir_init() very early in the process of creating a new
> > objects, so we're not getting good names here:
> > 
> > The "name" in alloc_netdev_mqs is actually a format string, so we end
> > up with a name like "eth%d" here:
> > 
> > net/core/dev.c: ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);
>  
> 
> 
> 
> Oh, yes, never thought about that...
> 
> It also seems like it might be a common problem, the subsystem defined
> name has not been given yet, although the core dev_name(dev) should
> work. But in netdev drivers, alloc_netdev_mqs() does not have access
> to that.
> 
> > At the point that we call these (preinit), net->ns.inum hasn't been
> > assigned yet, so we can't incorporate it properly into the dentry name:
> > 
> > net/core/net_namespace.c:       ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > net/core/net_namespace.c:       ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> > 
> > We could do the ref_tracker_dir_init() calls later, but we may end up
> > missing some references that way (or end up crashing because the "dir"
> > isn't initialized.
> > 
> > My current thinking is to add a new ref_tracker_dir_finalize() function
> > that we could use to finalize the name and register the debugfs files
> > after we have the requisite info. It's an extra manual step, but I like
> > that better than moving around the ref_tracker_dir_init() calls.
> 
> Agree. Although, bike shedding, i don't really like the _finalize in
> ref_tracker_dir_finalize(). Ideally this should be optional and
> populate debugfs.  _finalize does not sound particularly optional. So
> maybe ref_tracker_dir_debugfs()?
> 

I'm terrible at naming, so we can go with that. That last step will
definitely be optional.

> This also has the advantage of we need to worry less about GPU
> drivers. They see no change in behaviour, but we can give them a heads
> up the new call exists if they want to use it to populate debugfs.
> 

+1
-- 
Jeff Layton <jlayton@...nel.org>