Message-ID: <202008251055.7F5DD6FAEB@keescook>
Date:   Tue, 25 Aug 2020 11:27:09 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        linux-kernel@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Rob Herring <robh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [PATCH] MAINTAINERS: add namespace entry

On Tue, Aug 25, 2020 at 11:26:07AM -0500, Eric W. Biederman wrote:
> B) The challenge is that most of the namespace work has become part of
>    it's upstream subsystem so we really need to list the containers
>    list and ourselves as reviewers, more than maintainers who run
>    a tree for the code.

As a person who has to touch multiple subsystems regularly while doing
treewide changes, I'm WAY happier to have a distinct set of maintainers
for specific files, and I can track the patch acceptance because I see
them appearing in a specific tree (via -next, etc).

> C) You have overstated what I have agreed to here.
>    I have have previously said that I agree that having a MAINTAINERS
>    entry so people who are unfamiliar with the situation with namespaces
>    can find us.  Given that most of the changes going forward are likely
>    to be maintenance changes.
> 
>    I also said we need to talk about how we plan to maintain the code
>    here.
> 
>    It feels like you are pushing this hard, and I am not certain why you
>    are pushing and rushing this.  With my maintainer hat on my big
>    concern is we catch the issues that will introduce security issue.
>    Recently I have seen a report that there is an issue on Ubuntu
>    kernels where anyone can read /etc/shadow.  The problem is that
>    Ubuntu has not been cautions and has not taken the time to figure out
>    how to enable things for unprivileged users safely, and have just
>    enabled the code to be used by unprivileged users because it is
>    useful.
> 
>    In combination with you pushing hard and not taking the time to
>    complete this conversation in private with me, this MAINTAINERS entry
>    makes me uneasy as it feels like you may be looking for a way to push
>    the code into the mainline kernel like has been pushed into the
>    Ubuntu kernel.  I may be completely wrong I just don't know what to
>    make of your not finishing our conversation in private, and forcing
>    my hand by posting this patch publicly.

Eh? I don't see a conspiracy here; I think you are, as you suggest above,
completely wrong. ;) I haven't seen the private emails, obviously, but
what I see here is just Christian's drive to get things nailed down. It
sounds like the "here's who to CC" part of the MAINTAINERS file was
agreed to, but there was a misunderstanding about the resolution of
group maintainership?

> The files you have listed are reasonable for a maintainers entry as they
> have no other maintainers.

Agreed; if this was in place a few years ago, it might have been a bit
easier to direct and land some of the (now straggling) refcount_t
conversion patches.

> I know I have been less active after the birth of my young son, and I
> know the practical rule is that the person who does the work is the
> maintainer.  At the same time I am not convinced you are actually going
> to do the work to make new code maintainable and not a problem for other
> kernel developers.

O_O  I find this opinion very surprising. I hold Christian's judgement
in high regard (and yours). He's tackled the pidfd API (which solves so
many ancient gnarly problems with pid management) in a clean, measured,
and consistent manner. He and Aleksa have diligently worked on extensible
syscalls, which solve years of headaches over code maintainability, and
Christian regularly adds kernel selftests. I think he's absolutely got the
best interests of other developers (and users) in mind. Certainly none
of us are perfect, but your statement feels way way off base to me.

> A big part the job over the years has been to make the namespace ideas
> proposed sane, and to keep the burden from other maintainers of naive
> and terrible code.  Pushing this change before we finished our private
> conversation makes me very nervous on that front.

I think you both want the same thing (generally awesome Linux,
specifically sane and safe namespaces), and I think you're both deeply
involved in the namespace code and the use-cases, so it seems natural
to me that you'd have some form of shared maintainership. To that end, I
hope this can get sorted out. I'd like to have a tree I can count on to
have patches reviewed (and hopefully landed) that touch these areas.

To me, it seems like there has just been an impedance mismatch on the
expectations/priorities of communication speed. I don't see bad
motivations here at all.

> > [...]
> > +T:     https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/
> > +T:     https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git/

Obviously y'all still need to finish this discussion, I but I'd expect a
single tree at the end of the day. No other subsystem has multiple trees
that I can find:

$ git grep -A1 ^T: -- MAINTAINERS  | grep -- -T: | wc -l
0

And please use the "git" format for T: so you can specify branches,
which helps immensely in tracking down "latest" commits:

T: git git://URL/TREE.git BRANCH

No other trees use a bare https schema:

$ git grep ^T: -- MAINTAINERS | grep "git " | wc -l
632
$ git grep ^T: -- MAINTAINERS | grep -v "git "
MAINTAINERS:T:  quilt http://people.redhat.com/agk/patches/linux/editing/
MAINTAINERS:T:  quilt http://jdelvare.nerim.net/devel/linux/jdelvare-dmi/
MAINTAINERS:T:  hg http://tboot.hg.sourceforge.net:8000/hgroot/tboot/tboot
MAINTAINERS:T:  quilt https://ozlabs.org/~akpm/mmotm/
MAINTAINERS:T:  quilt https://ozlabs.org/~akpm/mmots/
MAINTAINERS:T:  git://git.infradead.org/nvme.git
MAINTAINERS:T:  git://git.infradead.org/nvme.git


-- 
Kees Cook

Powered by blists - more mailing lists