Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Giuseppe Scrivano <gscrivan@redhat.com>
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     ebiederm@xmission.com, "Serge E. Hallyn" <serge@hallyn.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] userns: automatically split user namespace extent
References: <20201203150252.1229077-1-gscrivan@redhat.com>
        <20210510172351.GA19918@mail.hallyn.com>
        <20210510185715.GA20897@mail.hallyn.com> <87h7idbskw.fsf@redhat.com>
        <20210605130016.jdkkviwtuefocset@wittgenstein>
Date:   Mon, 07 Jun 2021 11:31:37 +0200
In-Reply-To: <20210605130016.jdkkviwtuefocset@wittgenstein> (Christian
        Brauner's message of "Sat, 5 Jun 2021 15:00:16 +0200")
Message-ID: <874keaaume.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Fri, Jun 04, 2021 at 04:41:19PM +0200, Giuseppe Scrivano wrote:
>> Christian, Eric,
>> 
>> are you fine with this patch or is there anything more you'd like me to
>> change?
>
> Before being a little bit of a party pooper thanks for your patches! I
> appreciate the work you're doing!
>
> So my concern is that this may cause silent regressions/security issues
> for tools in userspace by making this work automagically.
>
> For example we have a go library that calculates idmap ranges and
> extents. Those idmappings are stored in the database and in the
> container's config and for backups and so on.
>
> The calculated extents match exactly with how these lines look in
> /proc/<pid>/*id_map.
> If we miscalculate the extents and we try to write them to
> /proc/<pid>/*id_map we get told to go away and immediately recognize the
> bug.
> With this patch however we may succeed and then we record misleading
> extents in the db or the config.
>
> Turning this into a general concern, I think it is a non-trivial
> semantic change to break up the 1:1 correspondence between mappings
> written and mappings applied that we had for such a long time now.
>
> In general I'm not sure it should be the kernel that has the idmapping
> ranges smarts.
>
> I'd rather see a generic userspace library that container runtimes make
> use of that also breaks up idmapping ranges. We can certainly accomodate
> this in
> https://pkg.go.dev/github.com/lxc/lxd/shared/idmap
>
> Is that a reasonable concern?

I've ended up adding a similar logic to Podman for the same reason as
above.

In our use case, containers are created within a user namespace that
usually has two extents, the current unprivileged ID mapped to root,
and any additional ID allocated to the user through /etc/sub?id mapped
to 1.

Within this user namespace, other user namespaces can be created and we
let users specify the mappings.  It is a common mistake to specify a
mapping that overlaps multiple extents in the parent userns e.g:
0:0:IDS_AVAILABLE.

To avoid the problem we have to first parse /proc/self/?id_map and then
split the specified extents when they overlap.

In our case this is not an issue anymore, moving the logic to the kernel
would just avoid a open syscall.

IMHO the 1:1 mapping is just an implementation detail, that is not
obvious for users.  Having the split in the kernel will also avoid that
this same check is added to each container runtimes that uses nested
user namespaces.

Thanks,
Giuseppe