On Wed, 2021-12-08 at 15:46 +0100, Christian Brauner wrote:

On Wed, Dec 08, 2021 at 09:11:09AM -0500, James Bottomley wrote:

quoted

On Wed, 2021-12-08 at 13:58 +0100, Christian Brauner wrote:

quoted

On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote:

[...]

quoted

quoted

@@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct

fs_context *fc)
 
 static void securityfs_kill_super(struct super_block *sb)
 {
+	struct user_namespace *ns = sb->s_fs_info;
+
+	if (ns != &init_user_ns)
+		ima_fs_ns_free_dentries(ns);

Say securityfs is unmounted. Then all the inodes and dentries
become invalid. It's not allowed to hold on to any dentries or
inodes after the super_block is shut down. So I just want to be
sure that nothing in ima can access these dentries after
securityfs is unmounted.

To put it another way: why are they stored in struct
ima_namespace in the first place? If you don't pin a filesystem
when creating files or directories like you do for securityfs in
init_ima_ns then you don't need to hold on to them as they will
be automatically be wiped during umount.

For IMA this is true because IMA can't be a module.  However, a
modular

This thread is about ima and its stashing of dentries in struct
ima_namespace. That things might be different for other consumers is
uninteresting for this specific case, I think.

Well, yes, but the patch series also includes namespacing securityfs. 
We have to get that right for all consumers, including the modular
ones.  So I think the way it works is we don't need a remove callback
in kill_sb() if we don't raise the dentry refcount in create.  However,
we still need to return the dentry to allow for stashing and we still
need to be able to cope with remove being called for the namespaced
entries ... for teardown on error in the IMA case and module
removal+teardown on error in other cases.

quoted

consumer, like the TPM, must be able to remove its entries from a
mounted securityfs because the code that serves the operations is
going away.  In order to do this removal, it needs the dentries
somewhere. 

That still doesn't require you to take an additional reference on the
dentry per se.

No, I don't believe it does ... however it does require a stash
somewhere.

Aside from this brings in a whole different and way bigger issue as
that requires way more fundamental work since this is about a (pseudo
or proper) device. It's not even clear that this should have entries
outside of init_user_ns-securityfs.

adding vTPMs is a use case that people want to support, so I don't want
to get to that bit and find it all doesn't work.

quoted

The current convention seems to be everything has a directory in
the top level, so we could call d_genocide() on this directory and
not have to worry about storing the dentries underneath, but I
think we can't avoid storing the dentry for the top level
directory.

I have not heard an argument why ima needs to stash these dentries as
it doesn't remove them once created until umount.

I'm not saying IMA does, I'm just saying we still need the dentry
returned by the API in case a consumer does need to stash.

James