Message-ID: <f408c42b74e28d90fce262abd50fc8a3079c0fa3.camel@HansenPartnership.com>
Subject: Re: [PATCH v7 01/14] ima: Add IMA namespace support
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Christian Brauner <christian.brauner@ubuntu.com>,
        Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc:     linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.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,
        Stefan Berger <stefanb@linux.ibm.com>
Date:   Thu, 16 Dec 2021 16:52:47 -0500
In-Reply-To: <20211216140806.hi4jxw54yvbu5cox@wittgenstein>
References: <20211216054323.1707384-1-stefanb@linux.vnet.ibm.com>
         <20211216054323.1707384-2-stefanb@linux.vnet.ibm.com>
         <20211216140806.hi4jxw54yvbu5cox@wittgenstein>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2021-12-16 at 15:08 +0100, Christian Brauner wrote:
> On Thu, Dec 16, 2021 at 12:43:10AM -0500, Stefan Berger wrote:
[...]
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index 6b2e3ca7ee99..6fa01323aac9 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/fs_struct.h>
> >  #include <linux/bsearch.h>
> >  #include <linux/sort.h>
> > +#include <linux/ima.h>
> >  
> >  static struct kmem_cache *user_ns_cachep __read_mostly;
> >  static DEFINE_MUTEX(userns_state_mutex);
> > @@ -141,8 +142,14 @@ int create_user_ns(struct cred *new)
> >  	if (!setup_userns_sysctls(ns))
> >  		goto fail_keyring;
> >  
> > +	ret = create_ima_ns(ns);
> 
> Instead of greedily allocating a new ima namespace for each new user
> namespace creation and wasting memory that is likely wasted since
> most containers won't use ima (for a long time at least) have you
> considered lazily allocating it like I suggested in one of my first
> reviews?
> 
> So under the assumption that the only way for a container to get its
> own ima policy it needs to have mounted a new securityfs instance you
> can move the ima namespace allocation into
> fill_super/ima_fs_ns_init():

The current patch set has the ima namespace born with an empty policy,
meaning it can never do anything until a new policy is inserted via a
write to the securityfs, and therefore the IMA namespace could be
lazily allocated.  However, that's not quite how the initial IMA
namespace behaves because a policy can be passed in on the kernel
command line (or built into the kernel).  If the ima NS were born with
a default policy (say taken from the initial IMA default policy, or
simply inherited from the parent at creation time) then we wouldn't be
able to do lazy allocation.  Before we tie ourselves to never being
able to have a default policy for an IMA namespace, perhaps we should
discuss if this is the correct behaviour we want to nail into the
system.

James