Message-ID: <0d6d0a22-0f3a-5f99-e603-f139d8fe7801@linux.ibm.com>
Date:   Fri, 17 Dec 2021 21:38:16 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v7 00/14] ima: Namespace IMA with audit support in IMA-ns
Content-Language: en-US
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, jejb@linux.ibm.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
References: <20211216054323.1707384-1-stefanb@linux.vnet.ibm.com>
 <20211216125027.fte6625wu5vxkjpi@wittgenstein>
 <20211216133148.aw3xs4sxuebkampb@wittgenstein>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20211216133148.aw3xs4sxuebkampb@wittgenstein>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 12/16/21 08:31, Christian Brauner wrote:
>
> 1. namespace securityfs
>     This patch is thematically standalone and should move to the
>     beginning of the series.
>     I would strongly recommend to fold patch 9 and 10 into a single patch
>     and add a lengthy explanation. You should be able to recycle a lof of
>     stuff I wrote in earlier reviews.
>
> 2. Introduce struct ima_namespace and pass it through to all callers:
>     - introduce struct ima_namespace
>     - move all the relevant things into this structure (this also avoids
>       the "avoid_zero_size" hack).


We could defer the kmalloc() that doesn't work on a zero-sized request. 
I would say this  is minor.


>     - define, setup, and expose init_ima_ns
>     - introduce get_current_ns() and always have it return &init_ima_ns for now
>     - replace all accesses to global variables to go through &init_ima_ns
>     - add new infrastructure you'll need later on
>     Bonus is that you can extend all the functions that later need access
>     to a specific ima namespace to take a struct ima_namespace * argument
>     and pass down &init_ima_ns down (retrieved via get_current_ns()). This
>     will make the actual namespace patch very easy to follow.
>
> 3. namespace ima
>     - add a new entry for struct ima_namespace to struct user_namespace
>     - add creation helpers, kmem cache etc.
>     - create files in securityfs per ns

I have tried this now and I am looking at 4 remaining patches that need 
to somehow find its way into v8 without causing too many disturbances. 
At what point (over how many patches) can I introduce CONFIG_IMA_NS 
without anything related to IMA namespacing happening? I need it early 
in 'your 3rd part' since it is also used for conditional compilation 
(Makefile) and #ifdef's where Makefile content and what the #ifdefs are 
doing probably shouldn't be squeezed into a single patch just so it's 
all enabled in one patch, but it should probably still remain logically 
separated into different patches. Enablement of IMA namespace would be 
in the very last patch. But there may be several patches between the 
very last one and CONFIG_IMA_NS is introduced...

v7 at least, before the requirement to do late/lazy initialization, 
enabled CONFIG_IMA_NS right away and built ever step on top of it, even 
if the IMA namespace only became **configurable** in the last patch when 
securityfs was enbled and one could set a policy. From that perspective 
it would be easier to switch to late initialization in a patch on top of 
v7 but .. ok, we cannot do that.


> This way at all points in the series we have clearly defined semantics
> where ima namespacing is either fully working or fully not working and
> the switch is atomic in the patch(es) part of 3.
Atomic over multiple patches? So introducing CONFIG_IMA_NS that doesn't 
do anything for several patches is still considered 'atomic' then ?