Re: [PATCH v8 01/19] securityfs: Extend securityfs with namespacing support

[Christian Brauner]

On Tue, Jan 11, 2022 at 07:16:26AM -0500, Mimi Zohar wrote:
> On Wed, 2022-01-05 at 11:18 +0100, Christian Brauner wrote:
> > On Wed, Jan 05, 2022 at 03:58:11AM +0000, Al Viro wrote:
> > > On Tue, Jan 04, 2022 at 12:03:58PM -0500, Stefan Berger wrote:
> > > > From: Stefan Berger <stefanb@xxxxxxxxxxxxx> 
> 
> > > > Drop the additional dentry reference to enable simple cleanup of dentries
> > > > upon umount. Now the dentries do not need to be explicitly freed anymore
> > > > but we can just rely on d_genocide() and the dcache shrinker to do all
> > > > the required work.
> > 
> > The "additional dentry reference" mentioned only relates to an afaict
> > unnecessary dget() in securityfs_create_dentry() which I pointed out
> > as part of earlier reviews. But the phrasing implies that there's a
> > behavioral change for the initial securityfs instance based on the
> > removal of this additional dget() when there really isn't.
> > 
> > After securityfs_create_dentry() has created a new dentry via
> > lookup_one_len() and eventually called d_instantiate() it currently
> > takes an additional reference on the newly created dentry via dget().
> > This additional reference is then paired with an additional dput() in
> > securityfs_remove(). I have not yet seen a reason why this is
> > necessary maybe you can help there.
> > 
> > For example, contrast this with debugfs which has the same underlying
> > logic as securityfs, i.e. any created dentry pins the whole filesystem
> > via simple_pin_fs() until the dentry is released and simple_unpin_fs()
> > is called. It uses a similar pairing as securityfs: where securityfs
> > has the securityfs_create_dentry() and securityfs_remove() pairing,
> > debugfs has the __debugfs_create_file() and debugfs_remove() pairing.
> > But debugfs doesn't take an additional reference on the just created
> > dentry in __debugfs_create_file() which would need to be put in
> > debugfs_remove().
> > 
> > So if we contrast the creation routines of securityfs and debugfs directly
> > condensed to just the dentry references:
> > 
> > securityfs       |   debugfs
> > ---------------- | ------------------
> >                  |
> > lookup_one_len() |   lookup_one_len()
> > d_instantiate()  |   d_instantiate() 
> > dget()           |
> > 
> > And I have not understood why securityfs would need that additional
> > dget(). Not just intrinsically but also when contrasted with debugfs. So
> > that additional dget() is removed as part of this patch.
> 
> Assuming it isn't needed, could removing it be a separate patch and
> upstreamed independently of either the securityfs or IMA namespacing
> changes?

Yeah, if the security tree wants to take it. So sm like: