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

[Mimi Zohar <zohar@linux.ibm.com>]

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@linux.ibm.com> 

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

thanks,

Mimi

> 
> But the explanation in the commit message isn't ideal as it implies
> the removal of the additional dget() would have any impact on the
> pinning logic for securityfs when it does not.
> 
> But the pinning logic doesn't make sense outside of the initial
> namespace which can never go away and there are security modules that
> have files or settings for the whole system that never go away and will
> always keep the filesystem around.
> 
> But for unprivileged/userns containers that mount their own securityfs
> instance we want the securityfs instance cleaned up when it is
> unmounted. There is no need to duplicate the pinning logic or make the
> global securityfs instance display different information based on the
> userns. Both options would be really messy and hacky.
> 
> Instead we can simply give each userns it's own securityfs instance
> similar to how each ipc ns has its own mqueue instance and all entries
> in there are cleaned up on umount or when the whole container is
> shutdown. After the container is shutdown all of the security module
> settings for the container go away with it anyway. So for that we don't
> want any filesystem pinning done in securityfs_create_dentry(). And we
> also really don't want the additional dget() that is currently taken in
> securityfs_create_dentry() as it would pointlessly require us to dput()
> during superblock shutdown afaict. None of this however should cause any
> behavioral changes for the initial securityfs instance.