* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 16:58 UTC (permalink / raw)
To: David Howells
Cc: Christian Brauner, Jann Horn, khlebnikov, Andy Lutomirski,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, nagarathnam.muthusamy, Aleksa Sarai,
Al Viro, Joel Fernandes
In-Reply-To: <18683.1553532974@warthog.procyon.org.uk>
On Mon, Mar 25, 2019 at 9:56 AM David Howells <dhowells@redhat.com> wrote:
> Daniel Colascione <dancol@google.com> wrote:
>
> > System calls are cheap.
>
> Only to a point. x86_64 will have an issue when we hit syscall 512. We're
> currently at 427.
IIRC, a while ago, someone proposed restarting system call numbering
above (again, IIRC) 1024 for both 32- and 64-bit varieties to
establish a clean slate and sidestep this problem.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Konstantin Khlebnikov @ 2019-03-25 17:05 UTC (permalink / raw)
To: Daniel Colascione, Christian Brauner
Cc: Jann Horn, Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, nagarathnam.muthusamy, Aleksa Sarai,
Al Viro, Joel Fernandes
In-Reply-To: <CAKOZueuJSAiKU7fZR2FNDKKCktdyE-sADtWprpsNMAqYQvD9Jw@mail.gmail.com>
On 25.03.2019 19:48, Daniel Colascione wrote:
> On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.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. Multiple people have asked when this patchset will be sent in
>> for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
>> Muthusamy from Oracle [3].
>>
>> The intention of the original translate_pid() syscall was twofold:
>> 1. Provide translation of pids between pid namespaces
>> 2. Provide implicit pid namespace introspection
>>
>> Both functionalities are preserved. The latter task has been improved
>> upon though. In the original version of the pachset passing pid as 1
>> would allow to deterimine the relationship between the pid namespaces.
>> This is inherhently racy. If pid 1 inside a pid namespace has died it
>> would report false negatives. For example, if pid 1 inside of the target
>> pid namespace already died, it would report that the target pid
>> namespace cannot be reached from the source pid namespace because it
>> couldn't find the pid inside of the target pid namespace and thus
>> falsely report to the user that the two pid namespaces are not related.
>> This problem is simple to avoid. In the new version we simply walk the
>> list of ancestors and check whether the namespace are related to each
>> other. By doing it this way we can reliably report what the relationship
>> between two pid namespace file descriptors looks like.
>>
>> Additionally, this syscall has been extended to allow the retrieval of
>> pidfds independent of procfs. These pidfds can e.g. be used with the new
>> pidfd_send_signal() syscall we recently merged. The ability to retrieve
>> pidfds independent of procfs had already been requested in the
>> pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
>> [5]. A use-case where a kernel is compiled without procfs but where
>> pidfds are still useful has been outlined by Andy in [6]. Regular
>> anon-inode based file descriptors are used that stash a reference to
>> struct pid in file->private_data and drop that reference on close.
>>
>> With this translate_pid() has three closely related but still distinct
>> functionalities. To clarify the semantics and to make it easier for
>> userspace to use the syscall it has:
>> - gained a command argument and three commands clearly reflecting the
>> distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
>> PIDCMD_GET_PIDFD).
>> - been renamed to pidctl()
>
> Having made these changes, you've built a general-purpose command
> command multiplexer, not one operation that happens to be flexible.
> The general-purpose command multiplexer is a common antipattern:
> multiplexers make it hard to talk about different kernel-provided
> operations using the common vocabulary we use to distinguish
> kernel-related operations, the system call number. socketcall, for
> example, turned out to be cumbersome for users like SELinux policy
> writers. People had to do work work later to split socketcall into
> fine-grained system calls. Please split the pidctl system call so that
> the design is clean from the start and we avoid work later. System
> calls are cheap.
>
> Also, I'm still confused about how metadata access is supposed to work
> for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> You snipped out a portion of a previous email in which I asked about
> your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> place, we have two different kinds of file descriptors for processes,
> one derived from procfs and one that's independent. The former works
> with openat(2). The latter does not. To be very specific; if I'm
> writing a function that accepts a pidfd and I get a pidfd that comes
> from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> smaps or oom_score_adj or statm for the named process in a race-free
> manner?
>
Task metadata could be exposed via "pages" identified by offset:
struct pidfd_stats stats;
pread(pidfd, &stats, sizeof(stats), PIDFD_STATS_OFFSET);
I'm not sure that we need yet another binary procfs.
But it will be faster than current text-based for sure.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 17:07 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Christian Brauner, Jann Horn, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, nagarathnam.muthusamy, Aleksa Sarai,
Al Viro, Joel
In-Reply-To: <8075dfac-94d2-b8c5-e37a-afe9b88bb48e@yandex-team.ru>
On Mon, Mar 25, 2019 at 10:05 AM Konstantin Khlebnikov
<khlebnikov@yandex-team.ru> wrote:
> On 25.03.2019 19:48, Daniel Colascione wrote:
> > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.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. Multiple people have asked when this patchset will be sent in
> >> for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> >> Muthusamy from Oracle [3].
> >>
> >> The intention of the original translate_pid() syscall was twofold:
> >> 1. Provide translation of pids between pid namespaces
> >> 2. Provide implicit pid namespace introspection
> >>
> >> Both functionalities are preserved. The latter task has been improved
> >> upon though. In the original version of the pachset passing pid as 1
> >> would allow to deterimine the relationship between the pid namespaces.
> >> This is inherhently racy. If pid 1 inside a pid namespace has died it
> >> would report false negatives. For example, if pid 1 inside of the target
> >> pid namespace already died, it would report that the target pid
> >> namespace cannot be reached from the source pid namespace because it
> >> couldn't find the pid inside of the target pid namespace and thus
> >> falsely report to the user that the two pid namespaces are not related.
> >> This problem is simple to avoid. In the new version we simply walk the
> >> list of ancestors and check whether the namespace are related to each
> >> other. By doing it this way we can reliably report what the relationship
> >> between two pid namespace file descriptors looks like.
> >>
> >> Additionally, this syscall has been extended to allow the retrieval of
> >> pidfds independent of procfs. These pidfds can e.g. be used with the new
> >> pidfd_send_signal() syscall we recently merged. The ability to retrieve
> >> pidfds independent of procfs had already been requested in the
> >> pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> >> [5]. A use-case where a kernel is compiled without procfs but where
> >> pidfds are still useful has been outlined by Andy in [6]. Regular
> >> anon-inode based file descriptors are used that stash a reference to
> >> struct pid in file->private_data and drop that reference on close.
> >>
> >> With this translate_pid() has three closely related but still distinct
> >> functionalities. To clarify the semantics and to make it easier for
> >> userspace to use the syscall it has:
> >> - gained a command argument and three commands clearly reflecting the
> >> distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> >> PIDCMD_GET_PIDFD).
> >> - been renamed to pidctl()
> >
> > Having made these changes, you've built a general-purpose command
> > command multiplexer, not one operation that happens to be flexible.
> > The general-purpose command multiplexer is a common antipattern:
> > multiplexers make it hard to talk about different kernel-provided
> > operations using the common vocabulary we use to distinguish
> > kernel-related operations, the system call number. socketcall, for
> > example, turned out to be cumbersome for users like SELinux policy
> > writers. People had to do work work later to split socketcall into
> > fine-grained system calls. Please split the pidctl system call so that
> > the design is clean from the start and we avoid work later. System
> > calls are cheap.
> >
> > Also, I'm still confused about how metadata access is supposed to work
> > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > You snipped out a portion of a previous email in which I asked about
> > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > place, we have two different kinds of file descriptors for processes,
> > one derived from procfs and one that's independent. The former works
> > with openat(2). The latter does not. To be very specific; if I'm
> > writing a function that accepts a pidfd and I get a pidfd that comes
> > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > smaps or oom_score_adj or statm for the named process in a race-free
> > manner?
> >
>
> Task metadata could be exposed via "pages" identified by offset:
>
> struct pidfd_stats stats;
>
> pread(pidfd, &stats, sizeof(stats), PIDFD_STATS_OFFSET);
>
> I'm not sure that we need yet another binary procfs.
> But it will be faster than current text-based for sure.
There are many options. I have some detailed thoughts on several of
them, but long design emails seem rather unwelcome. Right now, I'd
like to hear how Christian intends his patch set to address this use
case.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidctl()
From: Mika Penttilä @ 2019-03-25 17:20 UTC (permalink / raw)
To: Christian Brauner, jannh@google.com, khlebnikov@yandex-team.ru,
luto@kernel.org, dhowells@redhat.com, serge@hallyn.com,
ebiederm@xmission.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: arnd@arndb.de, keescook@chromium.org, adobriyan@gmail.com,
tglx@linutronix.de, mtk.manpages@gmail.com, bl0pbl33p@gmail.com,
ldv@altlinux.org, akpm@linux-foundation.org, oleg@redhat.com,
nagarathnam.muthusamy@oracle.com, cyphar@cyphar.com,
viro@zeniv.linux.org.uk, joel@joelfernandes.org,
dancol@google.com
In-Reply-To: <20190325162052.28987-3-christian@brauner.io>
Hi!
> +SYSCALL_DEFINE5(pidctl, unsigned int, cmd, pid_t, pid, int, source, int, target,
> + unsigned int, flags)
> +{
> + struct pid_namespace *source_ns = NULL, *target_ns = NULL;
> + struct pid *struct_pid;
> + pid_t result;
> +
> + switch (cmd) {
> + case PIDCMD_QUERY_PIDNS:
> + if (pid != 0)
> + return -EINVAL;
> + pid = 1;
> + /* fall through */
> + case PIDCMD_QUERY_PID:
> + if (flags != 0)
> + return -EINVAL;
> + break;
> + case PIDCMD_GET_PIDFD:
> + if (flags & ~PIDCTL_CLOEXEC)
> + return -EINVAL;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + source_ns = get_pid_ns_by_fd(source);
> + result = PTR_ERR(source_ns);
> + if (IS_ERR(source_ns))
> + goto err_source;
> +
> + target_ns = get_pid_ns_by_fd(target);
> + result = PTR_ERR(target_ns);
> + if (IS_ERR(target_ns))
> + goto err_target;
> +
> + if (cmd == PIDCMD_QUERY_PIDNS) {
> + result = pidns_related(source_ns, target_ns);
> + } else {
> + rcu_read_lock();
> + struct_pid = find_pid_ns(pid, source_ns);
> + result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
Should you do get_pid(struct_pid) here to keep it alive till
pidfd_create_fd() ?
> + rcu_read_unlock();
> +
> + if (cmd == PIDCMD_GET_PIDFD) {
> + int cloexec = (flags & PIDCTL_CLOEXEC) ? O_CLOEXEC : 0;
> + if (result > 0)
> + result = pidfd_create_fd(struct_pid, cloexec);
> + else if (result == 0)
> + result = -ENOENT;
> + }
> + }
> +
> + if (target)
> + put_pid_ns(target_ns);
> +err_target:
> + if (source)
> + put_pid_ns(source_ns);
> +err_source:
> + return result;
> +}
> +
> void __init pid_idr_init(void)
> {
> /* Verify no one has done anything silly: */
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index aa6e72fb7c08..1c863fb3d55a 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -429,6 +429,31 @@ static struct ns_common *pidns_get_parent(struct ns_common *ns)
> return &get_pid_ns(pid_ns)->ns;
> }
>
> +/**
> + * pidnscmp - Determine if @ancestor is ancestor of @descendant
> + * @ancestor: pidns suspected to be the ancestor of @descendant
> + * @descendant: pidns suspected to be the descendant of @ancestor
> + *
> + * Returns -1 if @ancestor is not an ancestor of @descendant,
> + * 0 if @ancestor is the same pidns as @descendant, 1 if @ancestor
> + * is an ancestor of @descendant.
> + */
> +int pidnscmp(struct pid_namespace *ancestor, struct pid_namespace *descendant)
> +{
> + if (ancestor == descendant)
> + return 0;
> +
> + for (;;) {
> + if (!descendant)
> + return -1;
> + if (descendant == ancestor)
> + break;
> + descendant = descendant->parent;
> + }
> +
> + return 1;
> +}
> +
> static struct user_namespace *pidns_owner(struct ns_common *ns)
> {
> return to_pid_ns(ns)->user_ns;
^ permalink raw reply
* Re: [PATCH 01/17] fpga: dfl-fme-mgr: fix FME_PR_INTFC_ID register address.
From: Alan Tull @ 2019-03-25 17:28 UTC (permalink / raw)
To: Wu Hao; +Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api
In-Reply-To: <1553483264-5379-2-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
>
> FME_PR_INTFC_ID is used as compat_id for fpga manager and region,
> but high 64 bits and low 64 bits of the compat_id are swapped by
> mistake. This patch fixes this problem by fixing register address.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
Thanks,
Alan
> ---
> drivers/fpga/dfl-fme-mgr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index 76f3770..b3f7eee 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -30,8 +30,8 @@
> #define FME_PR_STS 0x10
> #define FME_PR_DATA 0x18
> #define FME_PR_ERR 0x20
> -#define FME_PR_INTFC_ID_H 0xA8
> -#define FME_PR_INTFC_ID_L 0xB0
> +#define FME_PR_INTFC_ID_L 0xA8
> +#define FME_PR_INTFC_ID_H 0xB0
>
> /* FME PR Control Register Bitfield */
> #define FME_PR_CTRL_PR_RST BIT_ULL(0) /* Reset PR engine */
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-25 17:36 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Jann Horn, khlebnikov, Andy Lutomirski,
David Howells, Serge E. Hallyn, Eric W. Biederman, Linux API,
linux-kernel, Arnd Bergmann, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, bl0pbl33p,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
nagarathnam.muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAKOZueuJSAiKU7fZR2FNDKKCktdyE-sADtWprpsNMAqYQvD9Jw@mail.gmail.com>
On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.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. Multiple people have asked when this patchset will be sent in
> > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > Muthusamy from Oracle [3].
> >
> > The intention of the original translate_pid() syscall was twofold:
> > 1. Provide translation of pids between pid namespaces
> > 2. Provide implicit pid namespace introspection
> >
> > Both functionalities are preserved. The latter task has been improved
> > upon though. In the original version of the pachset passing pid as 1
> > would allow to deterimine the relationship between the pid namespaces.
> > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > would report false negatives. For example, if pid 1 inside of the target
> > pid namespace already died, it would report that the target pid
> > namespace cannot be reached from the source pid namespace because it
> > couldn't find the pid inside of the target pid namespace and thus
> > falsely report to the user that the two pid namespaces are not related.
> > This problem is simple to avoid. In the new version we simply walk the
> > list of ancestors and check whether the namespace are related to each
> > other. By doing it this way we can reliably report what the relationship
> > between two pid namespace file descriptors looks like.
> >
> > Additionally, this syscall has been extended to allow the retrieval of
> > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > pidfds independent of procfs had already been requested in the
> > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > [5]. A use-case where a kernel is compiled without procfs but where
> > pidfds are still useful has been outlined by Andy in [6]. Regular
> > anon-inode based file descriptors are used that stash a reference to
> > struct pid in file->private_data and drop that reference on close.
> >
> > With this translate_pid() has three closely related but still distinct
> > functionalities. To clarify the semantics and to make it easier for
> > userspace to use the syscall it has:
> > - gained a command argument and three commands clearly reflecting the
> > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > PIDCMD_GET_PIDFD).
> > - been renamed to pidctl()
>
[snip]
> Also, I'm still confused about how metadata access is supposed to work
> for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> You snipped out a portion of a previous email in which I asked about
> your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> place, we have two different kinds of file descriptors for processes,
> one derived from procfs and one that's independent. The former works
> with openat(2). The latter does not. To be very specific; if I'm
> writing a function that accepts a pidfd and I get a pidfd that comes
> from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> smaps or oom_score_adj or statm for the named process in a race-free
> manner?
This is true, that such usecase will not be supportable. But the advantage
on the other hand, is that suchs "pidfd" can be made pollable or readable in
the future. Potentially allowing us to return exit status without a new
syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
with proc.
But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
can be translated into a "pidfd" using another syscall or even a node, like
/proc/pid/handle or something. I think this is what Christian suggested in
the previous threads.
And also for the translation the other way, add a syscall or modify
translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
to /proc/pid directory fds.
Should we work on patches for these? Please let us know if this idea makes
sense and thanks a lot for adding us to the review as well.
Best,
- Joel
^ permalink raw reply
* Re: [PATCH 02/17] fpga: dfl: fme: align PR buffer size per PR datawidth
From: Alan Tull @ 2019-03-25 17:50 UTC (permalink / raw)
To: Wu Hao; +Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Xu Yilun
In-Reply-To: <1553483264-5379-3-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
Looks good, one question below.
>
> Current driver checks if input bitstream file size is aligned or
> not per PR data width (default 32bits). It requires one additional
> step for end user when they generate the bitstream file, padding
> extra zeros to bitstream file to align its size per PR data width,
> but they don't have to as hardware will drop extra padding bytes
> automatically.
>
> In order to simplify the user steps, this patch aligns PR buffer
> size per PR data width in driver, to allow user to pass unaligned
> size bitstream files to driver.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> drivers/fpga/dfl-fme-pr.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
> index d9ca955..c1fb1fe 100644
> --- a/drivers/fpga/dfl-fme-pr.c
> +++ b/drivers/fpga/dfl-fme-pr.c
> @@ -74,6 +74,7 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> struct dfl_fme *fme;
> unsigned long minsz;
> void *buf = NULL;
> + size_t length;
> int ret = 0;
> u64 v;
>
> @@ -85,9 +86,6 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> if (port_pr.argsz < minsz || port_pr.flags)
> return -EINVAL;
>
> - if (!IS_ALIGNED(port_pr.buffer_size, 4))
> - return -EINVAL;
> -
> /* get fme header region */
> fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
> FME_FEATURE_ID_HEADER);
> @@ -103,7 +101,13 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> port_pr.buffer_size))
> return -EFAULT;
>
> - buf = vmalloc(port_pr.buffer_size);
> + /*
> + * align PR buffer per PR bandwidth, as HW ignores the extra padding
> + * data automatically.
> + */
> + length = ALIGN(port_pr.buffer_size, 4);
> +
> + buf = vmalloc(length);
Since it may not be completely filled, would it be worthwhile to alloc
a zero'ed buff?
Alan
> if (!buf)
> return -ENOMEM;
>
> @@ -140,7 +144,7 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> fpga_image_info_free(region->info);
>
> info->buf = buf;
> - info->count = port_pr.buffer_size;
> + info->count = length;
> info->region_id = port_pr.port_id;
> region->info = info;
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 17:53 UTC (permalink / raw)
To: Joel Fernandes
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <20190325173614.GB25975@google.com>
On Mon, Mar 25, 2019 at 10:36 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.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. Multiple people have asked when this patchset will be sent in
> > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > Muthusamy from Oracle [3].
> > >
> > > The intention of the original translate_pid() syscall was twofold:
> > > 1. Provide translation of pids between pid namespaces
> > > 2. Provide implicit pid namespace introspection
> > >
> > > Both functionalities are preserved. The latter task has been improved
> > > upon though. In the original version of the pachset passing pid as 1
> > > would allow to deterimine the relationship between the pid namespaces.
> > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > would report false negatives. For example, if pid 1 inside of the target
> > > pid namespace already died, it would report that the target pid
> > > namespace cannot be reached from the source pid namespace because it
> > > couldn't find the pid inside of the target pid namespace and thus
> > > falsely report to the user that the two pid namespaces are not related.
> > > This problem is simple to avoid. In the new version we simply walk the
> > > list of ancestors and check whether the namespace are related to each
> > > other. By doing it this way we can reliably report what the relationship
> > > between two pid namespace file descriptors looks like.
> > >
> > > Additionally, this syscall has been extended to allow the retrieval of
> > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > pidfds independent of procfs had already been requested in the
> > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > [5]. A use-case where a kernel is compiled without procfs but where
> > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > anon-inode based file descriptors are used that stash a reference to
> > > struct pid in file->private_data and drop that reference on close.
> > >
> > > With this translate_pid() has three closely related but still distinct
> > > functionalities. To clarify the semantics and to make it easier for
> > > userspace to use the syscall it has:
> > > - gained a command argument and three commands clearly reflecting the
> > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > PIDCMD_GET_PIDFD).
> > > - been renamed to pidctl()
> >
> [snip]
> > Also, I'm still confused about how metadata access is supposed to work
> > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > You snipped out a portion of a previous email in which I asked about
> > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > place, we have two different kinds of file descriptors for processes,
> > one derived from procfs and one that's independent. The former works
> > with openat(2). The latter does not. To be very specific; if I'm
> > writing a function that accepts a pidfd and I get a pidfd that comes
> > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > smaps or oom_score_adj or statm for the named process in a race-free
> > manner?
>
> This is true, that such usecase will not be supportable. But the advantage
> on the other hand, is that suchs "pidfd" can be made pollable or readable in
> the future. Potentially allowing us to return exit status without a new
> syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> with proc.
I don't like the idea of having one kind of pollfd be pollable and
another not. Such an interface would be confusing for users. If, as
you suggest below, we instead make the procfs-less FD the only thing
we call a "pidfd", then this proposal becomes less confusing and more
viable.
> But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> can be translated into a "pidfd" using another syscall or even a node, like
> /proc/pid/handle or something. I think this is what Christian suggested in
> the previous threads.
/proc/pid/handle, if I understand correctly, "translates" a
procfs-based pidfd to a non-pidfd one. The needed interface would have
to perform the opposite translation, providing a procfs directory for
a given pidfd.
> And also for the translation the other way, add a syscall or modify
> translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> to /proc/pid directory fds.
This approach would work, but there's one subtlety to take into
account: which procfs? You'd want to take, as inputs, 1) the procfs
root you want, and 2) the pidfd, this way you could return to the user
a directory FD in the right procfs.
> Should we work on patches for these? Please let us know if this idea makes
> sense and thanks a lot for adding us to the review as well.
I would strongly prefer that we not merge pidctl (or whatever it is)
without a story for metadata access, be it your suggestion or
something else.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidctl()
From: Jann Horn @ 2019-03-25 18:18 UTC (permalink / raw)
To: Christian Brauner, Andy Lutomirski
Cc: Konstantin Khlebnikov, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, kernel list, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy, cyphar,
Al Viro, Joel Fernandes (Google), Daniel
In-Reply-To: <20190325162052.28987-3-christian@brauner.io>
On Mon, Mar 25, 2019 at 5:21 PM Christian Brauner <christian@brauner.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:
[...]
> +
> +static struct pid_namespace *get_pid_ns_by_fd(int fd)
> +{
> + struct pid_namespace *pidns = ERR_PTR(-EINVAL);
> +
> + if (fd >= 0) {
> +#ifdef CONFIG_PID_NS
> + struct ns_common *ns;
> + struct file *file = proc_ns_fget(fd);
> + if (IS_ERR(file))
> + return ERR_CAST(file);
> +
> + ns = get_proc_ns(file_inode(file));
> + if (ns->ops->type == CLONE_NEWPID)
> + pidns = get_pid_ns(
> + container_of(ns, struct pid_namespace, ns));
This increments the refcount of the pidns...
> +
> + fput(file);
> +#endif
> + } else {
> + pidns = task_active_pid_ns(current);
... but this doesn't. That's pretty subtle; could you please put a
comment on top of this function that points this out? Or even better,
change the function to always take a reference, so that the caller
doesn't have to worry about figuring this out.
> + }
> +
> + return pidns;
> +}
[...]
> +SYSCALL_DEFINE5(pidctl, unsigned int, cmd, pid_t, pid, int, source, int, target,
> + unsigned int, flags)
> +{
> + struct pid_namespace *source_ns = NULL, *target_ns = NULL;
> + struct pid *struct_pid;
> + pid_t result;
> +
> + switch (cmd) {
> + case PIDCMD_QUERY_PIDNS:
> + if (pid != 0)
> + return -EINVAL;
> + pid = 1;
> + /* fall through */
> + case PIDCMD_QUERY_PID:
> + if (flags != 0)
> + return -EINVAL;
> + break;
> + case PIDCMD_GET_PIDFD:
> + if (flags & ~PIDCTL_CLOEXEC)
> + return -EINVAL;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + source_ns = get_pid_ns_by_fd(source);
> + result = PTR_ERR(source_ns);
I very much dislike using PTR_ERR() on pointers before checking
whether they contain an error value or not. I understand that the
result of this won't actually be used, but it still seems weird to
have what is essentially a cast of a potentially valid pointer to a
potentially smaller integer here.
Could you maybe move the PTR_ERR() into the error branch? Like so:
if (IS_ERR(source_ns)) {
result = PTR_ERR(source_ns);
goto err_source;
}
> + if (IS_ERR(source_ns))
> + goto err_source;
> +
> + target_ns = get_pid_ns_by_fd(target);
> + result = PTR_ERR(target_ns);
> + if (IS_ERR(target_ns))
> + goto err_target;
> +
> + if (cmd == PIDCMD_QUERY_PIDNS) {
> + result = pidns_related(source_ns, target_ns);
> + } else {
> + rcu_read_lock();
> + struct_pid = find_pid_ns(pid, source_ns);
find_pid_ns() doesn't take a reference on its return value, the return
value is only pinned into memory by the RCU read-side critical
section...
> + result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
> + rcu_read_unlock();
... which ends here, making struct_pid a dangling pointer...
> +
> + if (cmd == PIDCMD_GET_PIDFD) {
> + int cloexec = (flags & PIDCTL_CLOEXEC) ? O_CLOEXEC : 0;
> + if (result > 0)
> + result = pidfd_create_fd(struct_pid, cloexec);
... and then here you continue to use struct_pid. That seems bogus.
> + else if (result == 0)
> + result = -ENOENT;
You don't need to have flags for this for new syscalls, you can just
make everything O_CLOEXEC; if someone wants to preserve an fd across
execve(), they can just clear that bit with fcntl(). (I thiiink it was
Andy Lutomirski who said that before regarding another syscall? But my
memory of that is pretty foggy, might've been someone else.)
> + }
> + }
> +
> + if (target)
> + put_pid_ns(target_ns);
> +err_target:
> + if (source)
> + put_pid_ns(source_ns);
I think you probably intended to check for "if (target >= 0)" and "if
(source >= 0)" instead of these conditions, matching the condition in
get_pid_ns_by_fd()? The current code looks as if it will leave the
refcount one too high when used with target==0 or source==0, and leave
the refcount one too low when used with target==-1 or source==-1.
Anyway, as stated above, I think it would be simpler to
unconditionally take a reference instead.
> +err_source:
> + return result;
> +}
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 18:19 UTC (permalink / raw)
To: Daniel Colascione
Cc: Joel Fernandes, Christian Brauner, Jann Horn,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAKOZuetbZOs1Yur4OrW6hh3wVYdoKNkSqTszbpTWnuRBTmT9zA@mail.gmail.com>
On Mon, Mar 25, 2019 at 5:53 PM Daniel Colascione <dancol@google.com> wrote:
>
> [..snip..]
>
> I don't like the idea of having one kind of pollfd be pollable and
> another not. Such an interface would be confusing for users. If, as
> you suggest below, we instead make the procfs-less FD the only thing
> we call a "pidfd", then this proposal becomes less confusing and more
> viable.
That's certainly on the table, we remove the ability to open
/proc/<PID> and use the dir fd with pidfd_send_signal. I'm in favor of
this.
> > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > can be translated into a "pidfd" using another syscall or even a node, like
> > /proc/pid/handle or something. I think this is what Christian suggested in
> > the previous threads.
>
> /proc/pid/handle, if I understand correctly, "translates" a
> procfs-based pidfd to a non-pidfd one. The needed interface would have
> to perform the opposite translation, providing a procfs directory for
> a given pidfd.
>
> > And also for the translation the other way, add a syscall or modify
> > translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> > directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> > Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> > to /proc/pid directory fds.
>
> This approach would work, but there's one subtlety to take into
> account: which procfs? You'd want to take, as inputs, 1) the procfs
> root you want, and 2) the pidfd, this way you could return to the user
> a directory FD in the right procfs.
>
I don't think metadata access is in the scope of such a pidfd itself.
> > Should we work on patches for these? Please let us know if this idea makes
> > sense and thanks a lot for adding us to the review as well.
>
> I would strongly prefer that we not merge pidctl (or whatever it is)
> without a story for metadata access, be it your suggestion or
> something else.
IMO, this is out of scope for a process handle. Hence, the need for
metadata access musn't stall it as is.
If you really need this, the pid this pidfd is mapped to can be
exposed through fdinfo (which is perfectly fine for your case as you
use /proc anyway). This means you can reliably determine if you're
opening it for the same pid (and it hasn't been recycled) because this
pidfd will be pollable for state change (in the future). Exposing it
through fdinfo isn't a problem, pid ns is bound to the process during
its lifetime, it's a process creation time property, so the value
isn't going to change anyway. So you can do all metadata access you
want subsequently.
The upshot is that this same pidfd can then be returned from a future
extension of clone(2) Christian is planning to work on. It is still
undecided whether this fd should be only readable by the parent
process or everyone (which would just be a matter of changing the
access mode depending on the caller), but that we can discuss all that
when the patchset is posted.
^ permalink raw reply
* Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal()
From: Jonathan Kowalski @ 2019-03-25 18:28 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190325162052.28987-4-christian@brauner.io>
On Mon, Mar 25, 2019 at 4:21 PM Christian Brauner <christian@brauner.io> wrote:
>
> Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch
> pidfd_send_signal() becomes independent of procfs. This fullfils the
> request made when we merged the pidfd_send_signal() patchset. The
> pidfd_send_signal() syscall is now always available allowing for it to be
> used by users without procfs mounted or even users without procfs support
> compiled into the kernel.
>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> Reviewed-by: David Howells <dhowells@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jann Horn <jannh@google.com
> Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
> Cc: "Dmitry V. Levin" <ldv@altlinux.org>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> ---
> kernel/signal.c | 20 +++++++++-----------
> kernel/sys_ni.c | 3 ---
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index b7953934aa99..d77183be1677 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> return kill_something_info(sig, &info, pid);
> }
>
> -#ifdef CONFIG_PROC_FS
> /*
> * Verify that the signaler and signalee either are in the same pid namespace
> * or that the signaler's pid namespace is an ancestor of the signalee's pid
> @@ -3521,16 +3520,13 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> */
> static bool access_pidfd_pidns(struct pid *pid)
If it is agreed upon to remove the ability of using /proc/<PID> as a
pidfd pidfd_send_signal accepts, is lifting this check acceptable?
The system call as is does not allow for a process to acquire a pidfd
without being in an ancestor namespace or the same namespace. Thus,
there are good reasons to allow for this to work and be able to work
around the limitations imposed by pid namespaces if userspace
explicitly decides to do so, by passing around the pidfd to some other
process.
Also, you would need to translate this only once, when filling in the
structure. The other process is bound to its pid ns as long as it is
alive, therefore it would be simple without having to do anything
fancy at read side (unlike unix sockets).
> {
> + int ret;
> struct pid_namespace *active = task_active_pid_ns(current);
> struct pid_namespace *p = ns_of_pid(pid);
>
> - for (;;) {
> - if (!p)
> - return false;
> - if (p == active)
> - break;
> - p = p->parent;
> - }
> + ret = pidnscmp(active, p);
> + if (ret < 0)
> + return false;
>
> return true;
> }
> @@ -3581,12 +3577,15 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> if (flags)
> return -EINVAL;
>
> - f = fdget_raw(pidfd);
> + f = fdget(pidfd);
> if (!f.file)
> return -EBADF;
>
> /* Is this a pidfd? */
> - pid = tgid_pidfd_to_pid(f.file);
> + if (f.file->f_op == &pidfd_fops)
> + pid = f.file->private_data;
> + else
> + pid = tgid_pidfd_to_pid(f.file);
> if (IS_ERR(pid)) {
> ret = PTR_ERR(pid);
> goto err;
> @@ -3625,7 +3624,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> fdput(f);
> return ret;
> }
> -#endif /* CONFIG_PROC_FS */
>
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index d21f4befaea4..4d9ae5ea6caf 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -167,9 +167,6 @@ COND_SYSCALL(syslog);
>
> /* kernel/sched/core.c */
>
> -/* kernel/signal.c */
> -COND_SYSCALL(pidfd_send_signal);
> -
> /* kernel/sys.c */
> COND_SYSCALL(setregid);
> COND_SYSCALL(setgid);
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal()
From: Jann Horn @ 2019-03-25 18:39 UTC (permalink / raw)
To: Christian Brauner
Cc: Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, kernel list,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy, cyphar,
Al Viro, "Joel Fernandes (Google)" <joe>
In-Reply-To: <20190325162052.28987-4-christian@brauner.io>
On Mon, Mar 25, 2019 at 5:21 PM Christian Brauner <christian@brauner.io> wrote:
> Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch
> pidfd_send_signal() becomes independent of procfs. This fullfils the
> request made when we merged the pidfd_send_signal() patchset. The
> pidfd_send_signal() syscall is now always available allowing for it to be
> used by users without procfs mounted or even users without procfs support
> compiled into the kernel.
[...]
> static bool access_pidfd_pidns(struct pid *pid)
> {
> + int ret;
> struct pid_namespace *active = task_active_pid_ns(current);
> struct pid_namespace *p = ns_of_pid(pid);
>
> - for (;;) {
> - if (!p)
> - return false;
> - if (p == active)
> - break;
> - p = p->parent;
> - }
> + ret = pidnscmp(active, p);
> + if (ret < 0)
> + return false;
>
> return true;
> }
Nit, if we keep this function: "if (...) return false; return true;"
seems like an antipattern to me. How about "return ret >= 0", or even
"return pidnscmp(active, p) >= 0"?
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Alan Tull @ 2019-03-25 18:48 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <1553483264-5379-4-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
This looks fine.
>
> In early partial reconfiguration private feature, it only
> supports 32bit data width when writing data to hardware for
> PR. 512bit data width PR support is an important optimization
> for some specific solutions (e.g. XEON with FPGA integrated),
> it allows driver to use AVX512 instruction to improve the
> performance of partial reconfiguration. e.g. programming one
> 100MB bitstream image via this 512bit data width PR hardware
> only takes ~300ms, but 32bit revision requires ~3s per test
> result.
>
> Please note now this optimization is only done on revision 2
> of this PR private feature which is only used in integrated
> solution that AVX512 is always supported.
>
> Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 18:57 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Joel Fernandes, Christian Brauner, Jann Horn,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAGLj2rE1f4x36KFUjFLiFq8tF1RUmFuCSRTg_tCQ9W2G6LtizA@mail.gmail.com>
On Mon, Mar 25, 2019 at 11:19 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Mon, Mar 25, 2019 at 5:53 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > [..snip..]
> >
> > I don't like the idea of having one kind of pollfd be pollable and
> > another not. Such an interface would be confusing for users. If, as
> > you suggest below, we instead make the procfs-less FD the only thing
> > we call a "pidfd", then this proposal becomes less confusing and more
> > viable.
>
> That's certainly on the table, we remove the ability to open
> /proc/<PID> and use the dir fd with pidfd_send_signal. I'm in favor of
> this.
>
> > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > can be translated into a "pidfd" using another syscall or even a node, like
> > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > the previous threads.
> >
> > /proc/pid/handle, if I understand correctly, "translates" a
> > procfs-based pidfd to a non-pidfd one. The needed interface would have
> > to perform the opposite translation, providing a procfs directory for
> > a given pidfd.
> >
> > > And also for the translation the other way, add a syscall or modify
> > > translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> > > directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> > > Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> > > to /proc/pid directory fds.
> >
> > This approach would work, but there's one subtlety to take into
> > account: which procfs? You'd want to take, as inputs, 1) the procfs
> > root you want, and 2) the pidfd, this way you could return to the user
> > a directory FD in the right procfs.
> >
>
> I don't think metadata access is in the scope of such a pidfd itself.
It should be possible to write a race-free pkill(1) using pidfds.
Without metadata access tied to the pidfd somehow, that can't be done.
> > > Should we work on patches for these? Please let us know if this idea makes
> > > sense and thanks a lot for adding us to the review as well.
> >
> > I would strongly prefer that we not merge pidctl (or whatever it is)
> > without a story for metadata access, be it your suggestion or
> > something else.
>
> IMO, this is out of scope for a process handle. Hence, the need for
> metadata access musn't stall it as is.
On the contrary, rather than metadata being out of scope, metadata
access is essential. Given a file handle (an FD), you can learn about
the file backing that handle by using fstat(2). Given a socket handle
(a socket FD), you can learn about the socket with getsockopt(2) and
ioctl(2). It would be strange not to be able, similarly, to learn
things about a process given a handle (a pidfd) to that process. I
want process handles to be "boring" in that they let users query and
manipulate processes mostly like they manipulate other resources.
> If you really need this, the pid this pidfd is mapped to can be
> exposed through fdinfo (which is perfectly fine for your case as you
> use /proc anyway). This means you can reliably determine if you're
> opening it for the same pid (and it hasn't been recycled) because this
> pidfd will be pollable for state change (in the future). Exposing it
> through fdinfo isn't a problem, pid ns is bound to the process during
> its lifetime, it's a process creation time property, so the value
> isn't going to change anyway. So you can do all metadata access you
> want subsequently.
Thanks for the proposal. I'm a bit confused. Are you suggesting that
we report the numeric FD in fdinfo? Are you saying it should work
basically like this?
int get_oom_score_adj(int pidfd) {
unique_fd fdinfo_fd = open(fmt("/proc/self/fdinfo/%d", pidfd));
string fdinfo = read_all(fdinfo_fd);
numeric_pid = parse_fdinfo_for_line("pid");
unique_fd procdir_fd = open(fmt("/proc/%d", numeric_pid), O_DIRECTORY);
if(!is_pidfd_alive(pidfd)) { return -1; /* process died */ } /*
LIVENESS CHECK */
// We know the process named by pidfd was called NUMERIC_PID
// in our namespace (because fdinfo told us) and we know that the
named process
// was alive after we successfully opened its /proc/pid directory, therefore,
// PROCDIR_FD and PIDFD must refer to the same underlying process.
unique_fd oom_adj_score_fd = openat(procdir_fd, "oom_score_adj");
return parse_int(read_all(oom_adj_score_fd));
}
While this interface functions correctly, I have two concerns: 1) the
number of system calls necessary is very large -- by my count,
starting with the pifd, we need six, not counting the follow-on
close(2) calls (which would bring the total to nine[1]), and 2) it's
"fail unsafe": IMHO, most users in practice will skip the line marked
"LIVENESS CHECK", and as a result, their code will appear to work but
contain subtle race conditions. An explicit interface to translate
from a (PIDFD, PROCFS_ROOT) tuple to a /proc/pid directory file
descriptor would be both more efficient and fail-safe.
[1] as a separate matter, it'd be nice to have a batch version of close(2).
^ permalink raw reply
* Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal()
From: Christian Brauner @ 2019-03-25 19:41 UTC (permalink / raw)
To: Jann Horn
Cc: Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, kernel list,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy, cyphar,
Al Viro, "Joel Fernandes (Google)" <joe>
In-Reply-To: <CAG48ez1QSJTOb-DAQuVpa5feUe9K-pr8aYuyd=yXg0n9eSg-LQ@mail.gmail.com>
On Mon, Mar 25, 2019 at 07:39:25PM +0100, Jann Horn wrote:
> On Mon, Mar 25, 2019 at 5:21 PM Christian Brauner <christian@brauner.io> wrote:
> > Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch
> > pidfd_send_signal() becomes independent of procfs. This fullfils the
> > request made when we merged the pidfd_send_signal() patchset. The
> > pidfd_send_signal() syscall is now always available allowing for it to be
> > used by users without procfs mounted or even users without procfs support
> > compiled into the kernel.
> [...]
> > static bool access_pidfd_pidns(struct pid *pid)
> > {
> > + int ret;
> > struct pid_namespace *active = task_active_pid_ns(current);
> > struct pid_namespace *p = ns_of_pid(pid);
> >
> > - for (;;) {
> > - if (!p)
> > - return false;
> > - if (p == active)
> > - break;
> > - p = p->parent;
> > - }
> > + ret = pidnscmp(active, p);
> > + if (ret < 0)
> > + return false;
> >
> > return true;
> > }
>
> Nit, if we keep this function: "if (...) return false; return true;"
> seems like an antipattern to me. How about "return ret >= 0", or even
> "return pidnscmp(active, p) >= 0"?
Yip, sounds good.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 19:42 UTC (permalink / raw)
To: Daniel Colascione
Cc: Joel Fernandes, Christian Brauner, Jann Horn,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAKOZuetCFgu0B53+mGmQ3+539MPT_tiu-PACx2ATvihHrrmUKg@mail.gmail.com>
On Mon, Mar 25, 2019 at 6:57 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Mon, Mar 25, 2019 at 11:19 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 5:53 PM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > [..snip..]
> > >
> > > I don't like the idea of having one kind of pollfd be pollable and
> > > another not. Such an interface would be confusing for users. If, as
> > > you suggest below, we instead make the procfs-less FD the only thing
> > > we call a "pidfd", then this proposal becomes less confusing and more
> > > viable.
> >
> > That's certainly on the table, we remove the ability to open
> > /proc/<PID> and use the dir fd with pidfd_send_signal. I'm in favor of
> > this.
> >
> > > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > > can be translated into a "pidfd" using another syscall or even a node, like
> > > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > > the previous threads.
> > >
> > > /proc/pid/handle, if I understand correctly, "translates" a
> > > procfs-based pidfd to a non-pidfd one. The needed interface would have
> > > to perform the opposite translation, providing a procfs directory for
> > > a given pidfd.
> > >
> > > > And also for the translation the other way, add a syscall or modify
> > > > translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> > > > directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> > > > Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> > > > to /proc/pid directory fds.
> > >
> > > This approach would work, but there's one subtlety to take into
> > > account: which procfs? You'd want to take, as inputs, 1) the procfs
> > > root you want, and 2) the pidfd, this way you could return to the user
> > > a directory FD in the right procfs.
> > >
> >
> > I don't think metadata access is in the scope of such a pidfd itself.
>
> It should be possible to write a race-free pkill(1) using pidfds.
> Without metadata access tied to the pidfd somehow, that can't be done.
>
>
> > > > Should we work on patches for these? Please let us know if this idea makes
> > > > sense and thanks a lot for adding us to the review as well.
> > >
> > > I would strongly prefer that we not merge pidctl (or whatever it is)
> > > without a story for metadata access, be it your suggestion or
> > > something else.
> >
> > IMO, this is out of scope for a process handle. Hence, the need for
> > metadata access musn't stall it as is.
>
> On the contrary, rather than metadata being out of scope, metadata
> access is essential. Given a file handle (an FD), you can learn about
> the file backing that handle by using fstat(2). Given a socket handle
> (a socket FD), you can learn about the socket with getsockopt(2) and
> ioctl(2). It would be strange not to be able, similarly, to learn
> things about a process given a handle (a pidfd) to that process. I
> want process handles to be "boring" in that they let users query and
> manipulate processes mostly like they manipulate other resources.
>
Yes, but everything in /proc is not equivalent to an attribute, or an
option, and depending on its configuration, you may not want to allow
processes to even be able to see /proc for any PIDs other than those
running as their own user (hidepid). This means, even if this new
system call is added, to respect hidepid, it must, depending on if
/proc is mounted (and what hidepid is set to, and what gid= is set
to), return EPERM, because then there is a discrepancy between how the
two entrypoints to acquire a process handle do access control. This is
ugly, but ideally should work the way it is described above.
Otherwise, things would be able to bypass the restrictions made
therein, because we also want a system call.
PIDs however are addressable with kill(2) even with hidepid enabled.
For good reason, the capabilities you can exercise using a PID is
limited in that case. The same logic applies to a process reference
like pidfd.
I agree there should be some way (if you can take care of the hidepid
gotcha) to return a dir fd of /proc entry, but I don't think it should
be the pidfd itself. You can get it to work using the fdinfo thing for
now.
> > If you really need this, the pid this pidfd is mapped to can be
> > exposed through fdinfo (which is perfectly fine for your case as you
> > use /proc anyway). This means you can reliably determine if you're
> > opening it for the same pid (and it hasn't been recycled) because this
> > pidfd will be pollable for state change (in the future). Exposing it
> > through fdinfo isn't a problem, pid ns is bound to the process during
> > its lifetime, it's a process creation time property, so the value
> > isn't going to change anyway. So you can do all metadata access you
> > want subsequently.
>
> Thanks for the proposal. I'm a bit confused. Are you suggesting that
> we report the numeric FD in fdinfo? Are you saying it should work
> basically like this?
>
Yes, numeric PID that you would see from your namespace for the said
process the pidfd is for. It could be -1 if this process is not in any
of the namespaces (current or children) (which would happen, say if
you pass it to some other pidns residing process, during fd
installation on ithe target process we set that up properly).
> int get_oom_score_adj(int pidfd) {
> unique_fd fdinfo_fd = open(fmt("/proc/self/fdinfo/%d", pidfd));
> string fdinfo = read_all(fdinfo_fd);
> numeric_pid = parse_fdinfo_for_line("pid");
> unique_fd procdir_fd = open(fmt("/proc/%d", numeric_pid), O_DIRECTORY);
> if(!is_pidfd_alive(pidfd)) { return -1; /* process died */ } /*
> LIVENESS CHECK */
> // We know the process named by pidfd was called NUMERIC_PID
> // in our namespace (because fdinfo told us) and we know that the
> named process
> // was alive after we successfully opened its /proc/pid directory, therefore,
> // PROCDIR_FD and PIDFD must refer to the same underlying process.
> unique_fd oom_adj_score_fd = openat(procdir_fd, "oom_score_adj");
> return parse_int(read_all(oom_adj_score_fd));
> }
>
> While this interface functions correctly, I have two concerns: 1) the
> number of system calls necessary is very large -- by my count,
> starting with the pifd, we need six, not counting the follow-on
> close(2) calls (which would bring the total to nine[1]),
But hey, that's a bit rich if you're planning to collect metadata from
/proc ;-).
> and 2) it's
> "fail unsafe": IMHO, most users in practice will skip the line marked
> "LIVENESS CHECK", and as a result, their code will appear to work but
> contain subtle race conditions. An explicit interface to translate
> from a (PIDFD, PROCFS_ROOT) tuple to a /proc/pid directory file
> descriptor would be both more efficient and fail-safe.
>
> [1] as a separate matter, it'd be nice to have a batch version of close(2).
Since /proc is full of gunk, how about adding more to it and making
the magic symlink of /proc/self/fd for the pidfd to lead to the dirfd
of the /proc entry of the process it maps to, when one uses
O_DIRECTORY while opening it? Otherwise, it behaves as it does today.
It would be equivalent to opening the proc entry with usual access
restrictions (and hidepid made to work) but without the races, and
because for processes outside your and children pid ns, it shouldn't
work anyway, and since they wouldn't have their entry on this procfs
instance, it would all just fit in nicely?
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-25 19:58 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, Konstantin Khlebnikov, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, kernel list,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy, cyphar,
Al Viro, "Joel Fernandes (Google)" <joe>
In-Reply-To: <CAG48ez24xPM4f7ty=8k5p6_gVWd-bEUSxca5Ngy5svHgarhz+w@mail.gmail.com>
On Mon, Mar 25, 2019 at 07:18:42PM +0100, Jann Horn wrote:
> On Mon, Mar 25, 2019 at 5:21 PM Christian Brauner <christian@brauner.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:
> [...]
> > +
> > +static struct pid_namespace *get_pid_ns_by_fd(int fd)
> > +{
> > + struct pid_namespace *pidns = ERR_PTR(-EINVAL);
> > +
> > + if (fd >= 0) {
> > +#ifdef CONFIG_PID_NS
> > + struct ns_common *ns;
> > + struct file *file = proc_ns_fget(fd);
> > + if (IS_ERR(file))
> > + return ERR_CAST(file);
> > +
> > + ns = get_proc_ns(file_inode(file));
> > + if (ns->ops->type == CLONE_NEWPID)
> > + pidns = get_pid_ns(
> > + container_of(ns, struct pid_namespace, ns));
>
> This increments the refcount of the pidns...
I didn't touch that code. That's taken over from the orginial patchset.
Thanks for catching this.
>
> > +
> > + fput(file);
> > +#endif
> > + } else {
> > + pidns = task_active_pid_ns(current);
>
> ... but this doesn't. That's pretty subtle; could you please put a
> comment on top of this function that points this out? Or even better,
> change the function to always take a reference, so that the caller
> doesn't have to worry about figuring this out.
Always taking a reference sounds more correct. Will do.
>
> > + }
> > +
> > + return pidns;
> > +}
> [...]
> > +SYSCALL_DEFINE5(pidctl, unsigned int, cmd, pid_t, pid, int, source, int, target,
> > + unsigned int, flags)
> > +{
> > + struct pid_namespace *source_ns = NULL, *target_ns = NULL;
> > + struct pid *struct_pid;
> > + pid_t result;
> > +
> > + switch (cmd) {
> > + case PIDCMD_QUERY_PIDNS:
> > + if (pid != 0)
> > + return -EINVAL;
> > + pid = 1;
> > + /* fall through */
> > + case PIDCMD_QUERY_PID:
> > + if (flags != 0)
> > + return -EINVAL;
> > + break;
> > + case PIDCMD_GET_PIDFD:
> > + if (flags & ~PIDCTL_CLOEXEC)
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + source_ns = get_pid_ns_by_fd(source);
> > + result = PTR_ERR(source_ns);
>
> I very much dislike using PTR_ERR() on pointers before checking
> whether they contain an error value or not. I understand that the
> result of this won't actually be used, but it still seems weird to
> have what is essentially a cast of a potentially valid pointer to a
> potentially smaller integer here.
>
> Could you maybe move the PTR_ERR() into the error branch? Like so:
Will do!
>
> if (IS_ERR(source_ns)) {
> result = PTR_ERR(source_ns);
> goto err_source;
> }
>
> > + if (IS_ERR(source_ns))
> > + goto err_source;
> > +
> > + target_ns = get_pid_ns_by_fd(target);
> > + result = PTR_ERR(target_ns);
> > + if (IS_ERR(target_ns))
> > + goto err_target;
> > +
> > + if (cmd == PIDCMD_QUERY_PIDNS) {
> > + result = pidns_related(source_ns, target_ns);
> > + } else {
> > + rcu_read_lock();
> > + struct_pid = find_pid_ns(pid, source_ns);
>
> find_pid_ns() doesn't take a reference on its return value, the return
> value is only pinned into memory by the RCU read-side critical
> section...
>
> > + result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
> > + rcu_read_unlock();
>
> ... which ends here, making struct_pid a dangling pointer...
>
> > +
> > + if (cmd == PIDCMD_GET_PIDFD) {
> > + int cloexec = (flags & PIDCTL_CLOEXEC) ? O_CLOEXEC : 0;
> > + if (result > 0)
> > + result = pidfd_create_fd(struct_pid, cloexec);
>
> ... and then here you continue to use struct_pid. That seems bogus.
I'll just take a reference to struct pid once I found it to prevent that
from happening.
>
> > + else if (result == 0)
> > + result = -ENOENT;
>
> You don't need to have flags for this for new syscalls, you can just
> make everything O_CLOEXEC; if someone wants to preserve an fd across
> execve(), they can just clear that bit with fcntl(). (I thiiink it was
> Andy Lutomirski who said that before regarding another syscall? But my
> memory of that is pretty foggy, might've been someone else.)
If that's the way going forward this is fine with me!
>
> > + }
> > + }
> > +
> > + if (target)
> > + put_pid_ns(target_ns);
> > +err_target:
> > + if (source)
> > + put_pid_ns(source_ns);
>
> I think you probably intended to check for "if (target >= 0)" and "if
> (source >= 0)" instead of these conditions, matching the condition in
> get_pid_ns_by_fd()? The current code looks as if it will leave the
> refcount one too high when used with target==0 or source==0, and leave
> the refcount one too low when used with target==-1 or source==-1.
> Anyway, as stated above, I think it would be simpler to
> unconditionally take a reference instead.
Yep.
>
> > +err_source:
> > + return result;
> > +}
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-25 19:59 UTC (permalink / raw)
To: Mika Penttilä
Cc: jannh@google.com, khlebnikov@yandex-team.ru, luto@kernel.org,
dhowells@redhat.com, serge@hallyn.com, ebiederm@xmission.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
arnd@arndb.de, keescook@chromium.org, adobriyan@gmail.com,
tglx@linutronix.de, mtk.manpages@gmail.com, bl0pbl33p@gmail.com,
ldv@altlinux.org, akpm@linux-foundation.org
In-Reply-To: <071b12db-f84e-8666-d70a-1b6cce71f9c0@nextfour.com>
On Mon, Mar 25, 2019 at 05:20:54PM +0000, Mika Penttilä wrote:
> Hi!
>
>
> > +SYSCALL_DEFINE5(pidctl, unsigned int, cmd, pid_t, pid, int, source, int, target,
> > + unsigned int, flags)
> > +{
> > + struct pid_namespace *source_ns = NULL, *target_ns = NULL;
> > + struct pid *struct_pid;
> > + pid_t result;
> > +
> > + switch (cmd) {
> > + case PIDCMD_QUERY_PIDNS:
> > + if (pid != 0)
> > + return -EINVAL;
> > + pid = 1;
> > + /* fall through */
> > + case PIDCMD_QUERY_PID:
> > + if (flags != 0)
> > + return -EINVAL;
> > + break;
> > + case PIDCMD_GET_PIDFD:
> > + if (flags & ~PIDCTL_CLOEXEC)
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + source_ns = get_pid_ns_by_fd(source);
> > + result = PTR_ERR(source_ns);
> > + if (IS_ERR(source_ns))
> > + goto err_source;
> > +
> > + target_ns = get_pid_ns_by_fd(target);
> > + result = PTR_ERR(target_ns);
> > + if (IS_ERR(target_ns))
> > + goto err_target;
> > +
> > + if (cmd == PIDCMD_QUERY_PIDNS) {
> > + result = pidns_related(source_ns, target_ns);
> > + } else {
> > + rcu_read_lock();
> > + struct_pid = find_pid_ns(pid, source_ns);
> > + result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
>
> Should you do get_pid(struct_pid) here to keep it alive till
> pidfd_create_fd() ?
Yes, indeed. You and Jann both pointed this out! Thank you.
>
> > + rcu_read_unlock();
> > +
> > + if (cmd == PIDCMD_GET_PIDFD) {
> > + int cloexec = (flags & PIDCTL_CLOEXEC) ? O_CLOEXEC : 0;
> > + if (result > 0)
> > + result = pidfd_create_fd(struct_pid, cloexec);
> > + else if (result == 0)
> > + result = -ENOENT;
> > + }
> > + }
> > +
> > + if (target)
> > + put_pid_ns(target_ns);
> > +err_target:
> > + if (source)
> > + put_pid_ns(source_ns);
> > +err_source:
> > + return result;
> > +}
> > +
> > void __init pid_idr_init(void)
> > {
> > /* Verify no one has done anything silly: */
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index aa6e72fb7c08..1c863fb3d55a 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -429,6 +429,31 @@ static struct ns_common *pidns_get_parent(struct ns_common *ns)
> > return &get_pid_ns(pid_ns)->ns;
> > }
> >
> > +/**
> > + * pidnscmp - Determine if @ancestor is ancestor of @descendant
> > + * @ancestor: pidns suspected to be the ancestor of @descendant
> > + * @descendant: pidns suspected to be the descendant of @ancestor
> > + *
> > + * Returns -1 if @ancestor is not an ancestor of @descendant,
> > + * 0 if @ancestor is the same pidns as @descendant, 1 if @ancestor
> > + * is an ancestor of @descendant.
> > + */
> > +int pidnscmp(struct pid_namespace *ancestor, struct pid_namespace *descendant)
> > +{
> > + if (ancestor == descendant)
> > + return 0;
> > +
> > + for (;;) {
> > + if (!descendant)
> > + return -1;
> > + if (descendant == ancestor)
> > + break;
> > + descendant = descendant->parent;
> > + }
> > +
> > + return 1;
> > +}
> > +
> > static struct user_namespace *pidns_owner(struct ns_common *ns)
> > {
> > return to_pid_ns(ns)->user_ns;
^ permalink raw reply
* Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal()
From: Christian Brauner @ 2019-03-25 20:05 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAGLj2rGTK2xLhDhBjh1wSpAiuxbLBt-AwREB5kA8LeW4JFzgsg@mail.gmail.com>
On Mon, Mar 25, 2019 at 06:28:53PM +0000, Jonathan Kowalski wrote:
> On Mon, Mar 25, 2019 at 4:21 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch
> > pidfd_send_signal() becomes independent of procfs. This fullfils the
> > request made when we merged the pidfd_send_signal() patchset. The
> > pidfd_send_signal() syscall is now always available allowing for it to be
> > used by users without procfs mounted or even users without procfs support
> > compiled into the kernel.
> >
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > Reviewed-by: David Howells <dhowells@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Jann Horn <jannh@google.com
> > Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
> > Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
> > Cc: "Dmitry V. Levin" <ldv@altlinux.org>
> > Cc: Andy Lutomirsky <luto@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > kernel/signal.c | 20 +++++++++-----------
> > kernel/sys_ni.c | 3 ---
> > 2 files changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index b7953934aa99..d77183be1677 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> > return kill_something_info(sig, &info, pid);
> > }
> >
> > -#ifdef CONFIG_PROC_FS
> > /*
> > * Verify that the signaler and signalee either are in the same pid namespace
> > * or that the signaler's pid namespace is an ancestor of the signalee's pid
> > @@ -3521,16 +3520,13 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> > */
> > static bool access_pidfd_pidns(struct pid *pid)
>
> If it is agreed upon to remove the ability of using /proc/<PID> as a
> pidfd pidfd_send_signal accepts, is lifting this check acceptable?
>
> The system call as is does not allow for a process to acquire a pidfd
> without being in an ancestor namespace or the same namespace. Thus,
> there are good reasons to allow for this to work and be able to work
> around the limitations imposed by pid namespaces if userspace
> explicitly decides to do so, by passing around the pidfd to some other
> process.
The original problem with this was - I think - that we hadn't settled
what certain values in struct siginfo_t should be set to when signaling
e.g. into ancestor pid namespaces. And if we did, we need to hook into
a widely used function in the kernel (I don't have the appropriate
thread handy atm) and we decided to punt on this until userspace really
needs this feature.
>
> Also, you would need to translate this only once, when filling in the
> structure. The other process is bound to its pid ns as long as it is
> alive, therefore it would be simple without having to do anything
> fancy at read side (unlike unix sockets).
>
> > {
> > + int ret;
> > struct pid_namespace *active = task_active_pid_ns(current);
> > struct pid_namespace *p = ns_of_pid(pid);
> >
> > - for (;;) {
> > - if (!p)
> > - return false;
> > - if (p == active)
> > - break;
> > - p = p->parent;
> > - }
> > + ret = pidnscmp(active, p);
> > + if (ret < 0)
> > + return false;
> >
> > return true;
> > }
> > @@ -3581,12 +3577,15 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > if (flags)
> > return -EINVAL;
> >
> > - f = fdget_raw(pidfd);
> > + f = fdget(pidfd);
> > if (!f.file)
> > return -EBADF;
> >
> > /* Is this a pidfd? */
> > - pid = tgid_pidfd_to_pid(f.file);
> > + if (f.file->f_op == &pidfd_fops)
> > + pid = f.file->private_data;
> > + else
> > + pid = tgid_pidfd_to_pid(f.file);
> > if (IS_ERR(pid)) {
> > ret = PTR_ERR(pid);
> > goto err;
> > @@ -3625,7 +3624,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > fdput(f);
> > return ret;
> > }
> > -#endif /* CONFIG_PROC_FS */
> >
> > static int
> > do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > index d21f4befaea4..4d9ae5ea6caf 100644
> > --- a/kernel/sys_ni.c
> > +++ b/kernel/sys_ni.c
> > @@ -167,9 +167,6 @@ COND_SYSCALL(syslog);
> >
> > /* kernel/sched/core.c */
> >
> > -/* kernel/signal.c */
> > -COND_SYSCALL(pidfd_send_signal);
> > -
> > /* kernel/sys.c */
> > COND_SYSCALL(setregid);
> > COND_SYSCALL(setgid);
> > --
> > 2.21.0
> >
^ permalink raw reply
* Re: pidfd design
From: Jann Horn @ 2019-03-25 20:13 UTC (permalink / raw)
To: Andy Lutomirski, Christian Brauner
Cc: Daniel Colascione, Joel Fernandes, Suren Baghdasaryan,
Steven Rostedt, Sultan Alsawaf, Tim Murray, Michal Hocko,
Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov,
Serge E. Hallyn, Kees Cook
In-Reply-To: <CALCETrUFrFKC2YTLH7ViM_7XPYk3LNmNiaz6s8wtWo1pmJQXzg@mail.gmail.com>
On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, Mar 20, 2019 at 12:40 PM Daniel Colascione <dancol@google.com> wrote:
> > On Wed, Mar 20, 2019 at 12:14 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Wed, Mar 20, 2019 at 11:58:57AM -0700, Andy Lutomirski wrote:
> > > > On Wed, Mar 20, 2019 at 11:52 AM Christian Brauner <christian@brauner.io> wrote:
> > > > >
> > > > > You're misunderstanding. Again, I said in my previous mails it should
> > > > > accept pidfds optionally as arguments, yes. But I don't want it to
> > > > > return the status fds that you previously wanted pidfd_wait() to return.
> > > > > I really want to see Joel's pidfd_wait() patchset and have more people
> > > > > review the actual code.
> > > >
> > > > Just to make sure that no one is forgetting a material security consideration:
> > >
> > > Andy, thanks for commenting!
> > >
> > > >
> > > > $ ls /proc/self
> > > > attr exe mountinfo projid_map status
> > > > autogroup fd mounts root syscall
> > > > auxv fdinfo mountstats sched task
> > > > cgroup gid_map net schedstat timers
> > > > clear_refs io ns sessionid timerslack_ns
> > > > cmdline latency numa_maps setgroups uid_map
> > > > comm limits oom_adj smaps wchan
> > > > coredump_filter loginuid oom_score smaps_rollup
> > > > cpuset map_files oom_score_adj stack
> > > > cwd maps pagemap stat
> > > > environ mem personality statm
> > > >
> > > > A bunch of this stuff makes sense to make accessible through a syscall
> > > > interface that we expect to be used even in sandboxes. But a bunch of
> > > > it does not. For example, *_map, mounts, mountstats, and net are all
> > > > namespace-wide things that certain policies expect to be unavailable.
> > > > stack, for example, is a potential attack surface. Etc.
> >
> > If you can access these files sources via open(2) on /proc/<pid>, you
> > should be able to access them via a pidfd. If you can't, you
> > shouldn't. Which /proc? The one you'd get by mounting procfs. I don't
> > see how pidfd makes any material changes to anyone's security. As far
> > as I'm concerned, if a sandbox can't mount /proc at all, it's just a
> > broken and unsupported configuration.
>
> It's not "broken and unsupported". I know of an actual working,
> deployed container-ish sandbox that does exactly this. I would also
> guess that quite a few not-at-all-container-like sandboxes work like
> this. (The obvious seccomp + unshare + pivot_root
> deny-myself-access-to-lots-of-things trick results in no /proc, which
> is by dsign.)
>
> >
> > An actual threat model and real thought paid to access capabilities
> > would help. Almost everything around the interaction of Linux kernel
> > namespaces and security feels like a jumble of ad-hoc patches added as
> > afterthoughts in response to random objections.
>
> I fully agree. But if you start thinking for real about access
> capabilities, there's no way that you're going to conclude that a
> capability to access some process implies a capability to access the
> settings of its network namespace.
>
> >
> > >> All these new APIs either need to
> > > > return something more restrictive than a proc dirfd or they need to
> > > > follow the same rules.
> >
>
> ...
>
> > What's special about libraries? How is a library any worse-off using
> > openat(2) on a pidfd than it would be just opening the file called
> > "/proc/$apid"?
>
> Because most libraries actually work, right now, without /proc. Even
> libraries that spawn subprocesses. If we make the new API have the
> property that it doesn't work if you're in a non-root user namespace
> and /proc isn't mounted, the result will be an utter mess.
>
> >
> > > > Yes, this is unfortunate, but it is indeed the current situation. I
> > > > suppose that we could return magic restricted dirfds, or we could
> > > > return things that aren't dirfds and all and have some API that gives
> > > > you the dirfd associated with a procfd but only if you can see
> > > > /proc/PID.
> > >
> > > What would be your opinion to having a
> > > /proc/<pid>/handle
> > > file instead of having a dirfd. Essentially, what I initially proposed
> > > at LPC. The change on what we currently have in master would be:
> > > https://gist.github.com/brauner/59eec91550c5624c9999eaebd95a70df
> >
> > And how do you propose, given one of these handle objects, getting a
> > process's current priority, or its current oom score, or its list of
> > memory maps? As I mentioned in my original email, and which nobody has
> > addressed, if you don't use a dirfd as your process handle or you
> > don't provide an easy way to get one of these proc directory FDs, you
> > need to duplicate a lot of metadata access interfaces.
>
> An API that takes a process handle object and an fd pointing at /proc
> (the root of the proc fs) and gives you back a proc dirfd would do the
> trick. You could do this with no new kernel features at all if you're
> willing to read the pid, call openat(2), and handle the races in user
> code.
This seems like something that might be a good fit for two ioctls?
One ioctl on procfs roots to translate pidfds into that procfs,
subject to both the normal lookup permission checks and only working
if the pidfd has a translation into the procfs:
int proc_root_fd = open("/proc", O_RDONLY);
int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
int proc_pgid_fd = open("/proc/self", O_RDONLY);
int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
And then, as you proposed, the new sys_clone() can just return a
pidfd, and you can convert it into a procfs fd yourself if you want.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 20:14 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Joel Fernandes, Christian Brauner, Jann Horn,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAGLj2rHETs+FjkOVP6PJ_EiePP7FKiDwxaWyjwg1gMZPYpU1fQ@mail.gmail.com>
On Mon, Mar 25, 2019 at 12:42 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Mon, Mar 25, 2019 at 6:57 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 11:19 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 5:53 PM Daniel Colascione <dancol@google.com> wrote:
> > > >
> > > > [..snip..]
> > > >
> > > > I don't like the idea of having one kind of pollfd be pollable and
> > > > another not. Such an interface would be confusing for users. If, as
> > > > you suggest below, we instead make the procfs-less FD the only thing
> > > > we call a "pidfd", then this proposal becomes less confusing and more
> > > > viable.
> > >
> > > That's certainly on the table, we remove the ability to open
> > > /proc/<PID> and use the dir fd with pidfd_send_signal. I'm in favor of
> > > this.
> > >
> > > > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > > > can be translated into a "pidfd" using another syscall or even a node, like
> > > > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > > > the previous threads.
> > > >
> > > > /proc/pid/handle, if I understand correctly, "translates" a
> > > > procfs-based pidfd to a non-pidfd one. The needed interface would have
> > > > to perform the opposite translation, providing a procfs directory for
> > > > a given pidfd.
> > > >
> > > > > And also for the translation the other way, add a syscall or modify
> > > > > translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> > > > > directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> > > > > Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> > > > > to /proc/pid directory fds.
> > > >
> > > > This approach would work, but there's one subtlety to take into
> > > > account: which procfs? You'd want to take, as inputs, 1) the procfs
> > > > root you want, and 2) the pidfd, this way you could return to the user
> > > > a directory FD in the right procfs.
> > > >
> > >
> > > I don't think metadata access is in the scope of such a pidfd itself.
> >
> > It should be possible to write a race-free pkill(1) using pidfds.
> > Without metadata access tied to the pidfd somehow, that can't be done.
> >
> >
> > > > > Should we work on patches for these? Please let us know if this idea makes
> > > > > sense and thanks a lot for adding us to the review as well.
> > > >
> > > > I would strongly prefer that we not merge pidctl (or whatever it is)
> > > > without a story for metadata access, be it your suggestion or
> > > > something else.
> > >
> > > IMO, this is out of scope for a process handle. Hence, the need for
> > > metadata access musn't stall it as is.
> >
> > On the contrary, rather than metadata being out of scope, metadata
> > access is essential. Given a file handle (an FD), you can learn about
> > the file backing that handle by using fstat(2). Given a socket handle
> > (a socket FD), you can learn about the socket with getsockopt(2) and
> > ioctl(2). It would be strange not to be able, similarly, to learn
> > things about a process given a handle (a pidfd) to that process. I
> > want process handles to be "boring" in that they let users query and
> > manipulate processes mostly like they manipulate other resources.
> >
>
> Yes, but everything in /proc is not equivalent to an attribute, or an
> option, and depending on its configuration, you may not want to allow
> processes to even be able to see /proc for any PIDs other than those
> running as their own user (hidepid). This means, even if this new
> system call is added, to respect hidepid, it must, depending on if
> /proc is mounted (and what hidepid is set to, and what gid= is set
> to), return EPERM, because then there is a discrepancy between how the
> two entrypoints to acquire a process handle do access control.
That's why I proposed that this translation mechanism accept a procfs
root directory --- so you'd specify *which* procfs you want and let
the kernel apply whatever hidepid access restrictions it wants.
[snip]
> PIDs however are addressable with kill(2) even with hidepid enabled.
> For good reason, the capabilities you can exercise using a PID is
> limited in that case. The same logic applies to a process reference
> like pidfd.
Sure. I'm not proposing a mechanism that would grant anyone additional
access to anything --- I'm just suggesting that we provide a way to
"directly" open a procfs dirfd instead of having people parse an
fdinfo text file.
> I agree there should be some way (if you can take care of the hidepid
> gotcha) to return a dir fd of /proc entry, but I don't think it should
> be the pidfd itself.
It's fine that pidfds aren't procfs directory FDs so long as there's a
race-free way to go from the former to the latter.
> You can get it to work using the fdinfo thing for
> now.
>
> > > If you really need this, the pid this pidfd is mapped to can be
> > > exposed through fdinfo (which is perfectly fine for your case as you
> > > use /proc anyway). This means you can reliably determine if you're
> > > opening it for the same pid (and it hasn't been recycled) because this
> > > pidfd will be pollable for state change (in the future). Exposing it
> > > through fdinfo isn't a problem, pid ns is bound to the process during
> > > its lifetime, it's a process creation time property, so the value
> > > isn't going to change anyway. So you can do all metadata access you
> > > want subsequently.
> >
> > Thanks for the proposal. I'm a bit confused. Are you suggesting that
> > we report the numeric FD in fdinfo? Are you saying it should work
> > basically like this?
> >
>
> Yes, numeric PID that you would see from your namespace for the said
> process the pidfd is for.
> It could be -1 if this process is not in any
> of the namespaces (current or children) (which would happen, say if
> you pass it to some other pidns residing process, during fd
> installation on ithe target process we set that up properly).
>
> > int get_oom_score_adj(int pidfd) {
> > unique_fd fdinfo_fd = open(fmt("/proc/self/fdinfo/%d", pidfd));
> > string fdinfo = read_all(fdinfo_fd);
> > numeric_pid = parse_fdinfo_for_line("pid");
> > unique_fd procdir_fd = open(fmt("/proc/%d", numeric_pid), O_DIRECTORY);
> > if(!is_pidfd_alive(pidfd)) { return -1; /* process died */ } /*
> > LIVENESS CHECK */
> > // We know the process named by pidfd was called NUMERIC_PID
> > // in our namespace (because fdinfo told us) and we know that the
> > named process
> > // was alive after we successfully opened its /proc/pid directory, therefore,
> > // PROCDIR_FD and PIDFD must refer to the same underlying process.
> > unique_fd oom_adj_score_fd = openat(procdir_fd, "oom_score_adj");
> > return parse_int(read_all(oom_adj_score_fd));
> > }
> >
> > While this interface functions correctly, I have two concerns: 1) the
> > number of system calls necessary is very large -- by my count,
> > starting with the pifd, we need six, not counting the follow-on
> > close(2) calls (which would bring the total to nine[1]),
>
> But hey, that's a bit rich if you're planning to collect metadata from
> /proc ;-).
Depends on which interface --- reading something like oom_score_adj is
pretty cheap.
> > and 2) it's
> > "fail unsafe": IMHO, most users in practice will skip the line marked
> > "LIVENESS CHECK", and as a result, their code will appear to work but
> > contain subtle race conditions. An explicit interface to translate
> > from a (PIDFD, PROCFS_ROOT) tuple to a /proc/pid directory file
> > descriptor would be both more efficient and fail-safe.
> >
> > [1] as a separate matter, it'd be nice to have a batch version of close(2).
>
> Since /proc is full of gunk,
People keep saying /proc is bad, but I haven't seen any serious
proposals for a clean replacement. :-)
> how about adding more to it and making
> the magic symlink of /proc/self/fd for the pidfd to lead to the dirfd
> of the /proc entry of the process it maps to, when one uses
> O_DIRECTORY while opening it? Otherwise, it behaves as it does today.
> It would be equivalent to opening the proc entry with usual access
> restrictions (and hidepid made to work) but without the races, and
> because for processes outside your and children pid ns, it shouldn't
> work anyway, and since they wouldn't have their entry on this procfs
> instance, it would all just fit in nicely?
Thanks. That'll work. It's a bit magical, but /proc/self/fd is magical
anyway, so that's okay.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-25 20:15 UTC (permalink / raw)
To: Joel Fernandes
Cc: Daniel Colascione, Jann Horn, khlebnikov, Andy Lutomirski,
David Howells, Serge E. Hallyn, Eric W. Biederman, Linux API,
linux-kernel, Arnd Bergmann, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, bl0pbl33p,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
nagarathnam.muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190325173614.GB25975@google.com>
On Mon, Mar 25, 2019 at 01:36:14PM -0400, Joel Fernandes wrote:
> On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.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. Multiple people have asked when this patchset will be sent in
> > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > Muthusamy from Oracle [3].
> > >
> > > The intention of the original translate_pid() syscall was twofold:
> > > 1. Provide translation of pids between pid namespaces
> > > 2. Provide implicit pid namespace introspection
> > >
> > > Both functionalities are preserved. The latter task has been improved
> > > upon though. In the original version of the pachset passing pid as 1
> > > would allow to deterimine the relationship between the pid namespaces.
> > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > would report false negatives. For example, if pid 1 inside of the target
> > > pid namespace already died, it would report that the target pid
> > > namespace cannot be reached from the source pid namespace because it
> > > couldn't find the pid inside of the target pid namespace and thus
> > > falsely report to the user that the two pid namespaces are not related.
> > > This problem is simple to avoid. In the new version we simply walk the
> > > list of ancestors and check whether the namespace are related to each
> > > other. By doing it this way we can reliably report what the relationship
> > > between two pid namespace file descriptors looks like.
> > >
> > > Additionally, this syscall has been extended to allow the retrieval of
> > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > pidfds independent of procfs had already been requested in the
> > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > [5]. A use-case where a kernel is compiled without procfs but where
> > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > anon-inode based file descriptors are used that stash a reference to
> > > struct pid in file->private_data and drop that reference on close.
> > >
> > > With this translate_pid() has three closely related but still distinct
> > > functionalities. To clarify the semantics and to make it easier for
> > > userspace to use the syscall it has:
> > > - gained a command argument and three commands clearly reflecting the
> > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > PIDCMD_GET_PIDFD).
> > > - been renamed to pidctl()
> >
> [snip]
> > Also, I'm still confused about how metadata access is supposed to work
> > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > You snipped out a portion of a previous email in which I asked about
> > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > place, we have two different kinds of file descriptors for processes,
> > one derived from procfs and one that's independent. The former works
> > with openat(2). The latter does not. To be very specific; if I'm
> > writing a function that accepts a pidfd and I get a pidfd that comes
> > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > smaps or oom_score_adj or statm for the named process in a race-free
> > manner?
>
> This is true, that such usecase will not be supportable. But the advantage
> on the other hand, is that suchs "pidfd" can be made pollable or readable in
> the future. Potentially allowing us to return exit status without a new
> syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> with proc.
>
> But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> can be translated into a "pidfd" using another syscall or even a node, like
> /proc/pid/handle or something. I think this is what Christian suggested in
> the previous threads.
Andy - and Jann who I just talked to - have proposed solutions for this.
Jann's idea is similar to what you suggested, Joel. You could e.g. do an
ioctl() handler for /proc that would give you a dirfd back for a given
pidfd. The advantage is that pidfd_clone() can then give back pidfds
without having to care in what procfs the process is supposed to live.
That makes things a lot easier. But pidfds for the general case should
be anon inodes. It's clean, it's simple and it is way more secure.
>
> And also for the translation the other way, add a syscall or modify
> translate_fd or something, to covert a anon_inode pidfd into a /proc/pid
> directory fd. Then the user is welcomed to do openat(2) on _that_ directory fd.
> Then we modify pidfd_send_signal to only send signals to pure pidfd fds, not
> to /proc/pid directory fds.
>
> Should we work on patches for these? Please let us know if this idea makes
> sense and thanks a lot for adding us to the review as well.
>
> Best,
>
> - Joel
^ permalink raw reply
* Re: pidfd design
From: Daniel Colascione @ 2019-03-25 20:23 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, Christian Brauner, Joel Fernandes,
Suren Baghdasaryan, Steven Rostedt, Sultan Alsawaf, Tim Murray,
Michal Hocko, Greg Kroah-Hartman, Arve Hjønnevåg,
Todd Kjos, Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov,
Serge E. Hallyn
In-Reply-To: <CAG48ez3VMjLJBC_F3BxC2sc2s-28NdsrUduR=jX66XH0w2O-Qg@mail.gmail.com>
On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
>
> On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> > On Wed, Mar 20, 2019 at 12:40 PM Daniel Colascione <dancol@google.com> wrote:
> > > On Wed, Mar 20, 2019 at 12:14 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Wed, Mar 20, 2019 at 11:58:57AM -0700, Andy Lutomirski wrote:
> > > > > On Wed, Mar 20, 2019 at 11:52 AM Christian Brauner <christian@brauner.io> wrote:
> > > > > >
> > > > > > You're misunderstanding. Again, I said in my previous mails it should
> > > > > > accept pidfds optionally as arguments, yes. But I don't want it to
> > > > > > return the status fds that you previously wanted pidfd_wait() to return.
> > > > > > I really want to see Joel's pidfd_wait() patchset and have more people
> > > > > > review the actual code.
> > > > >
> > > > > Just to make sure that no one is forgetting a material security consideration:
> > > >
> > > > Andy, thanks for commenting!
> > > >
> > > > >
> > > > > $ ls /proc/self
> > > > > attr exe mountinfo projid_map status
> > > > > autogroup fd mounts root syscall
> > > > > auxv fdinfo mountstats sched task
> > > > > cgroup gid_map net schedstat timers
> > > > > clear_refs io ns sessionid timerslack_ns
> > > > > cmdline latency numa_maps setgroups uid_map
> > > > > comm limits oom_adj smaps wchan
> > > > > coredump_filter loginuid oom_score smaps_rollup
> > > > > cpuset map_files oom_score_adj stack
> > > > > cwd maps pagemap stat
> > > > > environ mem personality statm
> > > > >
> > > > > A bunch of this stuff makes sense to make accessible through a syscall
> > > > > interface that we expect to be used even in sandboxes. But a bunch of
> > > > > it does not. For example, *_map, mounts, mountstats, and net are all
> > > > > namespace-wide things that certain policies expect to be unavailable.
> > > > > stack, for example, is a potential attack surface. Etc.
> > >
> > > If you can access these files sources via open(2) on /proc/<pid>, you
> > > should be able to access them via a pidfd. If you can't, you
> > > shouldn't. Which /proc? The one you'd get by mounting procfs. I don't
> > > see how pidfd makes any material changes to anyone's security. As far
> > > as I'm concerned, if a sandbox can't mount /proc at all, it's just a
> > > broken and unsupported configuration.
> >
> > It's not "broken and unsupported". I know of an actual working,
> > deployed container-ish sandbox that does exactly this. I would also
> > guess that quite a few not-at-all-container-like sandboxes work like
> > this. (The obvious seccomp + unshare + pivot_root
> > deny-myself-access-to-lots-of-things trick results in no /proc, which
> > is by dsign.)
> >
> > >
> > > An actual threat model and real thought paid to access capabilities
> > > would help. Almost everything around the interaction of Linux kernel
> > > namespaces and security feels like a jumble of ad-hoc patches added as
> > > afterthoughts in response to random objections.
> >
> > I fully agree. But if you start thinking for real about access
> > capabilities, there's no way that you're going to conclude that a
> > capability to access some process implies a capability to access the
> > settings of its network namespace.
> >
> > >
> > > >> All these new APIs either need to
> > > > > return something more restrictive than a proc dirfd or they need to
> > > > > follow the same rules.
> > >
> >
> > ...
> >
> > > What's special about libraries? How is a library any worse-off using
> > > openat(2) on a pidfd than it would be just opening the file called
> > > "/proc/$apid"?
> >
> > Because most libraries actually work, right now, without /proc. Even
> > libraries that spawn subprocesses. If we make the new API have the
> > property that it doesn't work if you're in a non-root user namespace
> > and /proc isn't mounted, the result will be an utter mess.
> >
> > >
> > > > > Yes, this is unfortunate, but it is indeed the current situation. I
> > > > > suppose that we could return magic restricted dirfds, or we could
> > > > > return things that aren't dirfds and all and have some API that gives
> > > > > you the dirfd associated with a procfd but only if you can see
> > > > > /proc/PID.
> > > >
> > > > What would be your opinion to having a
> > > > /proc/<pid>/handle
> > > > file instead of having a dirfd. Essentially, what I initially proposed
> > > > at LPC. The change on what we currently have in master would be:
> > > > https://gist.github.com/brauner/59eec91550c5624c9999eaebd95a70df
> > >
> > > And how do you propose, given one of these handle objects, getting a
> > > process's current priority, or its current oom score, or its list of
> > > memory maps? As I mentioned in my original email, and which nobody has
> > > addressed, if you don't use a dirfd as your process handle or you
> > > don't provide an easy way to get one of these proc directory FDs, you
> > > need to duplicate a lot of metadata access interfaces.
> >
> > An API that takes a process handle object and an fd pointing at /proc
> > (the root of the proc fs) and gives you back a proc dirfd would do the
> > trick. You could do this with no new kernel features at all if you're
> > willing to read the pid, call openat(2), and handle the races in user
> > code.
>
> This seems like something that might be a good fit for two ioctls?
As an aside, we had a long discussion about why fundamental facilities
like this should be system calls, not ioctls. I think the arguments
still apply.
> One ioctl on procfs roots to translate pidfds into that procfs,
> subject to both the normal lookup permission checks and only working
> if the pidfd has a translation into the procfs:
>
> int proc_root_fd = open("/proc", O_RDONLY);
> int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
>
> And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
>
> int proc_pgid_fd = open("/proc/self", O_RDONLY);
> int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
>
>
> And then, as you proposed, the new sys_clone() can just return a
> pidfd, and you can convert it into a procfs fd yourself if you want.
I think that's the consensus we reached on the other thread. The
O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
enough.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jann Horn @ 2019-03-25 20:34 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jonathan Kowalski, Joel Fernandes, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarat>
In-Reply-To: <CAKOZueuYMKdkt8BoSj1+p7=mpe2PA8r7xT5QiF6q3cTK+6HsAA@mail.gmail.com>
On Mon, Mar 25, 2019 at 9:15 PM Daniel Colascione <dancol@google.com> wrote:
> On Mon, Mar 25, 2019 at 12:42 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> > On Mon, Mar 25, 2019 at 6:57 PM Daniel Colascione <dancol@google.com> wrote:
[...]
> > Yes, but everything in /proc is not equivalent to an attribute, or an
> > option, and depending on its configuration, you may not want to allow
> > processes to even be able to see /proc for any PIDs other than those
> > running as their own user (hidepid). This means, even if this new
> > system call is added, to respect hidepid, it must, depending on if
> > /proc is mounted (and what hidepid is set to, and what gid= is set
> > to), return EPERM, because then there is a discrepancy between how the
> > two entrypoints to acquire a process handle do access control.
>
> That's why I proposed that this translation mechanism accept a procfs
> root directory --- so you'd specify *which* procfs you want and let
> the kernel apply whatever hidepid access restrictions it wants.
[...]
> > > and 2) it's
> > > "fail unsafe": IMHO, most users in practice will skip the line marked
> > > "LIVENESS CHECK", and as a result, their code will appear to work but
> > > contain subtle race conditions. An explicit interface to translate
> > > from a (PIDFD, PROCFS_ROOT) tuple to a /proc/pid directory file
> > > descriptor would be both more efficient and fail-safe.
> > >
> > > [1] as a separate matter, it'd be nice to have a batch version of close(2).
> >
> > Since /proc is full of gunk,
>
> People keep saying /proc is bad, but I haven't seen any serious
> proposals for a clean replacement. :-)
>
> > how about adding more to it and making
> > the magic symlink of /proc/self/fd for the pidfd to lead to the dirfd
> > of the /proc entry of the process it maps to, when one uses
> > O_DIRECTORY while opening it? Otherwise, it behaves as it does today.
> > It would be equivalent to opening the proc entry with usual access
> > restrictions (and hidepid made to work) but without the races, and
> > because for processes outside your and children pid ns, it shouldn't
> > work anyway, and since they wouldn't have their entry on this procfs
> > instance, it would all just fit in nicely?
>
> Thanks. That'll work. It's a bit magical, but /proc/self/fd is magical
> anyway, so that's okay.
Please don't do that. /proc/$pid/fd refers to the set of file
descriptors the process has open, and semantically doesn't have much
to do with the identity of the process. If you want to have a procfs
directory entry for getting a pidfd, please add a new entry. (Although
I don't see the point in adding a new procfs entry for this when you
could instead have an ioctl or syscall operating on the procfs
directory fd.)
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 20:40 UTC (permalink / raw)
To: Jann Horn
Cc: Daniel Colascione, Joel Fernandes, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <CAG48ez1ZVKgwfQDYT1k4pB4-8Y8Ywv12dabh5KFFxtKmT-e7Cw@mail.gmail.com>
On Mon, Mar 25, 2019 at 8:34 PM Jann Horn <jannh@google.com> wrote:
>
> [...SNIP...]
>
> Please don't do that. /proc/$pid/fd refers to the set of file
> descriptors the process has open, and semantically doesn't have much
> to do with the identity of the process. If you want to have a procfs
> directory entry for getting a pidfd, please add a new entry. (Although
> I don't see the point in adding a new procfs entry for this when you
> could instead have an ioctl or syscall operating on the procfs
> directory fd.)
There is no new entry. What I was saying (and I should have been
clearer) is that the existing entry for the fd when open'd with
O_DIRECTORY makes the kernel resolve the symlink to /proc/<PID> of the
process it maps to, so it would become:
int dirfd = open("/proc/self/fd/3", O_DIRECTORY|O_CLOEXEC);
This also means you cannot cross the filesystem boundry, the said
process needs to have a visible entry (which would mean hidepid= and
gid= based access controls are honored), and you can only open the
dirfd of a process in the current ns (as the PID will not map to an
existent process if the pidfd maps to a process not in the same or
children pid ns, in fdinfo it lists -1 in the pid field (we might not
even need fdinfo anymore)).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox