linux-kernel - Re: [PATCH v1 2/4] pid: add pidctl()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKOZueuXtWwC+U9-q3QG41p-fwjMxmujTUegmVkorLzFcY2-SA@mail.gmail.com>
Date:   Tue, 26 Mar 2019 09:50:28 -0700
From:   Daniel Colascione <dancol@...gle.com>
To:     Christian Brauner <christian@...uner.io>
Cc:     Jann Horn <jannh@...gle.com>,
        Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        Andy Lutomirski <luto@...nel.org>,
        David Howells <dhowells@...hat.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Linux API <linux-api@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Kees Cook <keescook@...omium.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Michael Kerrisk-manpages <mtk.manpages@...il.com>,
        Jonathan Kowalski <bl0pbl33p@...il.com>,
        "Dmitry V. Levin" <ldv@...linux.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Nagarathnam Muthusamy <nagarathnam.muthusamy@...cle.com>,
        Aleksa Sarai <cyphar@...har.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH v1 2/4] pid: add pidctl()

On Tue, Mar 26, 2019 at 9:44 AM Christian Brauner <christian@...uner.io> wrote:
>
> On Tue, Mar 26, 2019 at 09:38:31AM -0700, Daniel Colascione wrote:
> > On Tue, Mar 26, 2019 at 9:34 AM Christian Brauner <christian@...uner.io> wrote:
> > >
> > > On Tue, Mar 26, 2019 at 05:31:42PM +0100, Christian Brauner wrote:
> > > > On Tue, Mar 26, 2019 at 05:23:37PM +0100, Christian Brauner wrote:
> > > > > On Tue, Mar 26, 2019 at 09:17:07AM -0700, Daniel Colascione wrote:
> > > > > > Thanks for the patch.
> > > > > >
> > > > > > On Tue, Mar 26, 2019 at 8:55 AM Christian Brauner <christian@...uner.io> wrote:
> > > > > > >
> > > > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > > > syscall:
> > > > > >
> > > > > > We still haven't had a much-needed conversation about splitting this
> > > > > > system call into smaller logical operations. It's important that we
> > > > > > address this point before this patch is merged and becomes permanent
> > > > > > kernel ABI.
> > > > >
> > > > > I don't particularly mind splitting this into an additional syscall like
> > > > > e.g.  pidfd_open() but then we have - and yes, I know you'll say
> > > > > syscalls are cheap - translate_pid(), and pidfd_open(). What I like
> > > > > about this rn is that it connects both apis in a single syscall
> > > > > and allows pidfd retrieval across pid namespaces. So I guess we'll see
> > > > > what other people think.
> > > >
> > > > There's something to be said for
> > > >
> > > > pidfd_open(pid_t pid, int pidfd, unsigned int flags);
> > > >
> > > > /* get pidfd */
> > > > int pidfd = pidfd_open(1234, -1, 0);
> > > >
> > > > /* convert to procfd */
> > > > int procfd = pidfd_open(-1, 4, 0);
> > > >
> > > > /* convert to pidfd */
> > > > int pidfd = pidfd_open(4, -1, 0);
> > >
> > > probably rather:
> > >
> > > int pidfd = pidfd_open(-1, 4, PIDFD_TO_PROCFD);
> > > int procfd = pidfd_open(-1, 4, PROCFD_TO_PIDFD);
> > > int pidfd = pidfd_open(1234, -1, 0);
> >
> > These three operations look like three related but distinct functions
> > to me, and in the second case, the "pidfd_open" name is a bit of a
> > misnomer. IMHO, the presence of an "operation name" field in any API
> > is usually a good indication that we're looking at a family of related
> > APIs, not a single coherent operation.
>
> So I'm happy to accommodate the need for a clean api even though I
> disagree that what we have in pidctl() is unclean.
> But I will not start sending a pile of syscalls. There is nothing
> necessarily wrong to group related APIs together.

In the email I sent just now, I identified several specific technical
disadvantages arising from unnecessary grouping of system calls. We
have historical evidence in the form of socketcall that this grouping
tends to be regrettable. I don't recall your identifying any
offsetting technical advantages. Did I miss something?

> By these standards the
> new mount API would need to be like 30 different syscalls, same for
> keyring management.

Can you please point out the problem that would arise from splitting
the mount and keyring APIs this way? One could have made the same
argument about grouping socket operations, and this socket-operation
grouping ended up being a mistake.