Message-ID: <915b3be92544f6572f214f360a592708f46ad7e6.camel@linux.ibm.com>
Subject: Re: [RFC v2 19/19] ima: Setup securityfs for IMA namespace
From:   James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org
Cc:     zohar@linux.ibm.com, serge@hallyn.com,
        christian.brauner@ubuntu.com, containers@lists.linux.dev,
        dmitry.kasatkin@gmail.com, ebiederm@xmission.com,
        krzysztof.struczynski@huawei.com, roberto.sassu@huawei.com,
        mpeters@redhat.com, lhinds@redhat.com, lsturman@redhat.com,
        puiterwi@redhat.com, jamjoom@us.ibm.com,
        linux-kernel@vger.kernel.org, paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
Date:   Mon, 06 Dec 2021 09:11:06 -0500
In-Reply-To: <e4134d4f-5134-ade8-6428-3510ac940757@linux.ibm.com>
References: <20211203023118.1447229-1-stefanb@linux.ibm.com>
         <20211203023118.1447229-20-stefanb@linux.ibm.com>
         <df433bc52ca1e0408d48bbace4c34a573991f5ba.camel@linux.ibm.com>
         <6306b4e5-f26d-1704-6344-354eb5387abf@linux.ibm.com>
         <11b557b58de74828b1c16334a5fb52c4d3f6ad0f.camel@linux.ibm.com>
         <cd05433a-3630-e7f5-e144-ff766d7792fa@linux.ibm.com>
         <084e6522e1e778408cf6b3581d75f7349d336006.camel@linux.ibm.com>
         <e4134d4f-5134-ade8-6428-3510ac940757@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2021-12-06 at 09:03 -0500, Stefan Berger wrote:
> On 12/5/21 23:27, James Bottomley wrote:
> > On Fri, 2021-12-03 at 14:11 -0500, Stefan Berger wrote:
> > > On 12/3/21 13:50, James Bottomley wrote:
> > > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > [...]
> > > > > I suppose any late filesystem init callchain would have to be
> > > > > connected to the user_namespace somehow?
> > > >   
> > > > I don't think so; I think just moving some securityfs entries
> > > > into
> > > > the user_namespace and managing the notifier chain from within
> > > > securityfs will do for now.  [although I'd have to spec this
> > > > out in
> > > > code before I knew for sure].
> > > It doesn't have to be right in the user_namespace. The IMA
> > > namespace
> > > is  connected to the user namespace and holds the dentries now...
> > > 
> > > Please spec it out...
> > OK, this is what I have.  fill_super turned out to be a locking
> > nightmare, so I triggered it from free context instead (which
> > doesn't
> > have the once per keyed superblock property, so I added a flag in
> > the
> > user namespace).  I've got it to the point where the event is
> > triggered
> > on mount and unmount, so all the entries for the namespace are
> > added
> > when the filesystem is mounted and remove when it's
> > unmounted.  This
> > style of addition no longer needs the simple_pin_fs, because the
> > add/remove callbacks substitute (plus, if we pinned, the free_super
> > wouldn't trigger on unmount).  The default behaviour still does
> > pinning
> > and unpinning, but that can be keyed off the current
> > user_namespace.
> > 
> > This is all on top of your current series ... some of the functions
> > should probably be renamed, but I kept them to show how the code
> > was
> > migrating in this sketch.
> > 
> > James
> > 
> > ---
> > 
> >  From 59c45daa8698c66c3bcebfb194123977d548a9a6 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date: Sat, 4 Dec 2021 16:38:37 +0000
> > Subject: [PATCH] rework securityfs
> > 
> > ---
> > 
> > -
> > -static void _securityfs_remove(struct dentry *dentry,
> > -			       struct vfsmount **mount, int
> > *mount_count)
> > +void securityfs_remove(struct dentry *dentry)
> >   {
> >   	struct inode *dir;
> > +	struct user_namespace *ns = current_user_ns();
> 
> I had problems with this in this place. So I had to use use
> 
> struct user_namespace *user_ns = dentry->d_sb->s_user_ns;

Yes, I think that works ... the owner in the parent namespace could
actually unmount it, so keying off the user namespace it was mounted on
is definitely the correct form.

> I'll try to split up your patch and post a v3 with then. Or is it too
> early?

It's never too early to see what the series is shaping up as.  However,
I'm still not sure I got the right trigger for the SECURITYFS_NS_ADD
notifier, so that may still have to move ... or even that there isn't
some locking subtlety I missed in triggering SECURITY_NS_REMOVE from
kill_sb.

I also suspect Christian will want a pointer to the securityfs pieces
in struct user_namespace rather than discrete elements added directly.

James