* Re: extending wait4(2) or waitid(2) linux syscall
From: Paul Eggert @ 2018-11-16 16:03 UTC (permalink / raw)
To: Arnd Bergmann, Florian Weimer
Cc: H. Peter Anvin, Dmitry V. Levin, Albert ARIBAUD, GNU C Library,
Linux API
In-Reply-To: <CAK8P3a2kDbzzdHdPNaU9LBdLbdFejLYzx+Ntt4s5F=7u+scHsg@mail.gmail.com>
On 11/16/18 7:17 AM, Arnd Bergmann wrote:
> we'd want something like
>
> int wait9(...)
We're getting uncomfortably close to the Perlis limit.
"If you have a procedure with 10 parameters, you probably missed some."
Perlis AJ. Epigrams on programming. SIGPLAN Notices. 1982:17(9):7-13.
https://dx.doi.org/10.1145/947955.1083808
http://pu.inf.uni-tuebingen.de/users/klaeren/epigrams.html
^ permalink raw reply
* Re: [PATCH v3 0/4] bpf: allow zero-initialising hash map seed
From: Song Liu @ 2018-11-16 17:33 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
> On Nov 16, 2018, at 3:41 AM, Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Allow forcing the seed of a hash table to zero, for deterministic
> execution during benchmarking and testing.
>
> Changes from v2:
> * Change ordering of BPF_F_ZERO_SEED in linux/bpf.h
>
> Comments adressed from v1:
> * Add comment to discourage production use to linux/bpf.h
> * Require CAP_SYS_ADMIN
>
> Lorenz Bauer (4):
> bpf: allow zero-initializing hash map seed
> bpf: move BPF_F_QUERY_EFFECTIVE after map flags
> tools: sync linux/bpf.h
> tools: add selftest for BPF_F_ZERO_SEED
>
> include/uapi/linux/bpf.h | 9 ++--
> kernel/bpf/hashtab.c | 13 ++++-
> tools/include/uapi/linux/bpf.h | 13 +++--
> tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
> 4 files changed, 84 insertions(+), 19 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 0/4] bpf: allow zero-initialising hash map seed
From: Song Liu @ 2018-11-16 17:34 UTC (permalink / raw)
To: Lorenz Bauer
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
> On Nov 16, 2018, at 3:41 AM, Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Allow forcing the seed of a hash table to zero, for deterministic
> execution during benchmarking and testing.
>
> Changes from v2:
> * Change ordering of BPF_F_ZERO_SEED in linux/bpf.h
>
> Comments adressed from v1:
> * Add comment to discourage production use to linux/bpf.h
> * Require CAP_SYS_ADMIN
>
> Lorenz Bauer (4):
> bpf: allow zero-initializing hash map seed
> bpf: move BPF_F_QUERY_EFFECTIVE after map flags
> tools: sync linux/bpf.h
> tools: add selftest for BPF_F_ZERO_SEED
>
> include/uapi/linux/bpf.h | 9 ++--
> kernel/bpf/hashtab.c | 13 ++++-
> tools/include/uapi/linux/bpf.h | 13 +++--
> tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
> 4 files changed, 84 insertions(+), 19 deletions(-)
>
> --
> 2.17.1
>
For the series:
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: hpa @ 2018-11-16 18:46 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Dmitry V. Levin, Albert ARIBAUD, GNU C Library, Linux API
In-Reply-To: <CAK8P3a2bGYt1Z50eQsBv2pu_zd649SB1MjX-iWcZ-YnJ+Bhj1Q@mail.gmail.com>
On November 15, 2018 11:14:21 PM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Thu, Nov 15, 2018 at 7:38 AM <hpa@zytor.com> wrote:
>> On November 15, 2018 7:30:11 AM PST, "Dmitry V. Levin"
><ldv@altlinux.org> wrote:
>> >On Thu, Nov 15, 2018 at 06:39:03AM -0800, Arnd Bergmann wrote:
>> >> On Thu, Nov 15, 2018 at 6:05 AM Dmitry V. Levin wrote:
>> >> > On Thu, Apr 20, 2017 at 03:20:51PM +0200, Albert ARIBAUD wrote:
>
>> >2. The time precision provided by struct rusage returned by wait4(2)
>> >and waitid(2) is too low for syscall time counting (strace -c)
>nowadays,
>> >this can be observing by running in a row a simple command like
>"strace -c
>> >pwd".
>> >
>> >The fix is to return a more appropriate structure than struct rusage
>> >by the new pwait6(2)/pwaitid(2) syscall mentioned above, where
>> >struct timeval is replaced with struct timespec or even struct
>> >timespec64.
>>
>> Arnd: w.r.t. our previous discussion, this would seem to justify
>going to timespec(64) for these kind of cases.
>
>Ok, and I assume we want the same layout for getrusage(2) then, right?
>
> Arnd
Right
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: hpa @ 2018-11-16 18:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Dmitry V. Levin, Albert ARIBAUD, GNU C Library, Linux API
In-Reply-To: <CAK8P3a2bGYt1Z50eQsBv2pu_zd649SB1MjX-iWcZ-YnJ+Bhj1Q@mail.gmail.com>
On November 15, 2018 11:14:21 PM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Thu, Nov 15, 2018 at 7:38 AM <hpa@zytor.com> wrote:
>> On November 15, 2018 7:30:11 AM PST, "Dmitry V. Levin"
><ldv@altlinux.org> wrote:
>> >On Thu, Nov 15, 2018 at 06:39:03AM -0800, Arnd Bergmann wrote:
>> >> On Thu, Nov 15, 2018 at 6:05 AM Dmitry V. Levin wrote:
>> >> > On Thu, Apr 20, 2017 at 03:20:51PM +0200, Albert ARIBAUD wrote:
>
>> >2. The time precision provided by struct rusage returned by wait4(2)
>> >and waitid(2) is too low for syscall time counting (strace -c)
>nowadays,
>> >this can be observing by running in a row a simple command like
>"strace -c
>> >pwd".
>> >
>> >The fix is to return a more appropriate structure than struct rusage
>> >by the new pwait6(2)/pwaitid(2) syscall mentioned above, where
>> >struct timeval is replaced with struct timespec or even struct
>> >timespec64.
>>
>> Arnd: w.r.t. our previous discussion, this would seem to justify
>going to timespec(64) for these kind of cases.
>
>Ok, and I assume we want the same layout for getrusage(2) then, right?
>
> Arnd
Note that now we have to add one system calls for 64-but platforms. Thus, the right way to do that is to add those new system calls (e.g. waitidns()), and *only* provide those for 32-bit Y2038.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Alan Cox @ 2018-11-16 21:24 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Florian Weimer,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <20181111111143.GB4189@1wt.eu>
> I think the issue is a bit more complex :
> - linux doesn't support a single libc
> - glibc doesn't support a single OS
>
> In practice we all know (believe?) that both statements above are
> true but in practice 99% of the time there's a 1:1 relation between
> these two components.
The top linux C library is probably the Android one. Given the number
of containers now running Alpine and the number of embedded devices it's
probably a good fight going on for 2nd, 3rd and 4th. It is certainly not
a Linux/Glibc world any more.
Alan
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Arnd Bergmann @ 2018-11-17 1:50 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: Albert ARIBAUD, H. Peter Anvin, GNU C Library, Linux API
In-Reply-To: <20181115153008.GC2171@altlinux.org>
On Thu, Nov 15, 2018 at 7:30 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Thu, Nov 15, 2018 at 06:39:03AM -0800, Arnd Bergmann wrote:
> > On Thu, Nov 15, 2018 at 6:05 AM Dmitry V. Levin wrote:
> > > On Thu, Apr 20, 2017 at 03:20:51PM +0200, Albert ARIBAUD wrote:
>
> 1. strace needs a race-free invocation of wait4(2) or waitid(2)
> with a different signal mask, this cannot be achieved without
> an extended version of syscall, similar to pselect6(2) extension
> over select(2) and ppoll(2) extension over poll(2).
>
> Signal mask specification in linux requires two parameters:
> "const sigset_t *sigmask" and "size_t sigsetsize".
> Creating pwait6(2) as an extension of wait4(2) with two arguments
> is straightforward.
> Creating pwaitid(2) as an extension of waitid(2) that already has 5
> arguments would require an indirection similar to pselect6(2).
Getting back to this point: you could also do the same thing with
the CLONE_FD approach from Josh Triplett[1] or Casey Dahlin's
older waitfd() syscall, correct?
Neither of them got merged, bu t we could revisit the ideas if they
make it easier to solve your problem by calling ppoll().
Arnd
[1] https://lwn.net/Articles/638613/
[2] https://lore.kernel.org/lkml/49639EB8.40204@redhat.com/
^ permalink raw reply
* Re: [PATCH 1/3] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Y Song @ 2018-11-18 5:47 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181116125329.3974-2-lmb@cloudflare.com>
On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Use data_size_out as a size hint when copying test output to user space.
> A program using BPF_PERF_OUTPUT can compare its own buffer length with
> data_size_out after the syscall to detect whether truncation has taken
> place. Callers which so far did not set data_size_in are not affected.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> net/bpf/test_run.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index c89c22c49015..30c57b7f4ba4 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -74,8 +74,15 @@ static int bpf_test_finish(const union bpf_attr *kattr,
> {
> void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
> int err = -EFAULT;
> + u32 copy_size = size;
>
> - if (data_out && copy_to_user(data_out, data, size))
> + /* Clamp copy if the user has provided a size hint, but copy the full
> + * buffer if not to retain old behaviour.
> + */
> + if (kattr->test.data_size_out && copy_size > kattr->test.data_size_out)
> + copy_size = kattr->test.data_size_out;
> +
> + if (data_out && copy_to_user(data_out, data, copy_size))
> goto out;
> if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
> goto out;
if copy_size < size, maybe we should return -ENOSPC so user space is aware
of insufficient size and takes proper action? This behavior will then
be consistent
with BPF_PROG_QUERY subcommand where prog_cnt is the in/out parameter and
-ENOSPC is returned if kernel does not have enough space to copy out
the whole data.
Also, since data_size_out field now has more complex semantics, could you add
some comments in uapi/linux/bpf.h so it will be relatively clear from
uapi header
how this field will be used?
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 2/3] libbpf: require size hint in bpf_prog_test_run
From: Y Song @ 2018-11-18 5:53 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181116125329.3974-3-lmb@cloudflare.com>
On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Require size_out to be non-NULL if data_out is given. This prevents
> accidental overwriting of process memory after the output buffer.
>
> Adjust callers of bpf_prog_test_run to this behaviour.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> tools/lib/bpf/bpf.c | 4 +++-
> tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 03f9bcc4ef50..127a9aa6170e 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -403,10 +403,12 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> attr.test.data_in = ptr_to_u64(data);
> attr.test.data_out = ptr_to_u64(data_out);
> attr.test.data_size_in = size;
> + if (data_out)
> + attr.test.data_size_out = *size_out;
Maybe it is better to return error (-EINVAL) instead of segfault if
size_out is NULL?
we should try to avoid segfault inside the library. This will change
original API behavior, but
I think it is okay since it is in the user space.
> attr.test.repeat = repeat;
>
> ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
> - if (size_out)
> + if (data_out)
> *size_out = attr.test.data_size_out;
> if (retval)
> *retval = attr.test.retval;
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 2d3c04f45530..560d7527b86b 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -150,6 +150,7 @@ static void test_xdp(void)
> bpf_map_update_elem(map_fd, &key4, &value4, 0);
> bpf_map_update_elem(map_fd, &key6, &value6, 0);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
>
> @@ -158,6 +159,7 @@ static void test_xdp(void)
> "err %d errno %d retval %d size %d\n",
> err, errno, retval, size);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
> buf, &size, &retval, &duration);
> CHECK(err || retval != XDP_TX || size != 114 ||
> @@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
> return;
> }
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
>
> @@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
> "ipv4", "err %d errno %d retval %d size %d\n",
> err, errno, retval, size);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
> buf, &size, &retval, &duration);
> CHECK(err || retval != XDP_TX || size != 54,
> @@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
> goto out;
> bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
> CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||
> @@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
> "err %d errno %d retval %d size %d magic %x\n",
> err, errno, retval, size, *magic);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
> buf, &size, &retval, &duration);
> CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||
> @@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
> goto out;
> bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
> CHECK(err || retval != 1 || size != 54 ||
> @@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
> "err %d errno %d retval %d size %d magic %x\n",
> err, errno, retval, size, *magic);
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
> buf, &size, &retval, &duration);
> CHECK(err || retval != 1 || size != 74 ||
> @@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
> pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
> }
>
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
> if (err || retval || size != sizeof(pkt_v4) ||
> @@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
> err, errno, retval, size, iph->daddr);
>
> /* Queue is empty, program should return TC_ACT_SHOT */
> + size = sizeof(buf);
> err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> buf, &size, &retval, &duration);
> CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size
From: Y Song @ 2018-11-18 5:59 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181116125329.3974-4-lmb@cloudflare.com>
On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Make sure that bpf_prog_test_run returns the correct length
> in the size_out argument and that the kernel respects the
> output size hint.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 560d7527b86b..6ab98e10e86f 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -124,6 +124,39 @@ static void test_pkt_access(void)
> bpf_object__close(obj);
> }
>
> +static void test_output_size_hint(void)
> +{
> + const char *file = "./test_pkt_access.o";
> + struct bpf_object *obj;
> + __u32 retval, size, duration;
> + int err, prog_fd;
> + char buf[10];
> +
> + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> + if (err) {
> + error_cnt++;
> + return;
> + }
CHECK can also be used here.
if (CHECK(...)) {
goto done;
}
where label "done" is right before bpf_object__close.
> +
> + memset(buf, 0, sizeof(buf));
> +
> + size = 5;
> + err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> + buf, &size, &retval, &duration);
> + CHECK(err || retval, "run",
> + "err %d errno %d retval %d\n",
> + err, errno, retval);
> +
> + CHECK(size != sizeof(pkt_v4), "out_size",
> + "incorrect output size, want %lu have %u\n",
> + sizeof(pkt_v4), size);
> +
> + CHECK(buf[5] != 0, "overflow",
> + "prog_test_run ignored size hint\n");
> +
> + bpf_object__close(obj);
> +}
> +
> static void test_xdp(void)
> {
> struct vip key4 = {.protocol = 6, .family = AF_INET};
> @@ -1847,6 +1880,7 @@ int main(void)
> jit_enabled = is_jit_enabled();
>
> test_pkt_access();
> + test_output_size_hint();
> test_xdp();
> test_xdp_adjust_tail();
> test_l4lb_all();
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface
From: Y Song @ 2018-11-18 6:13 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
>
> Fix this by using bpf_attr.test.data_size_out as a size hint. The old
> behaviour is retained if size_hint is zero.
There is a slight change of user space behavior for this patch.
Without this patch, the value bpf_attr.test.data_size_out is output only.
For example,
output buffer : out_buf (user allocated size 10)
data_size_out is a random value (e.g., 1),
The actual data to copy is 5.
In today's implementation, the kernel will copy 5 and set data_size_out is 5.
With this patch, the kernel will copy 1 and set data_size_out is 5.
I am not 100% sure at this time whether we CAN overload data_size_out
since it MAY break existing applications.
Alternativley, we can append another field to bpf_attr.test
__u32 data_out_size;
this will provide the data_out buffer size.
Inside kernel, if the user input attr is smaller than kernel and does not
have data_out_size, the current behavior should be used. Otherwise,
data_out_size is data_out buffer size.
Daniel and Alexei, which option do you think is reasonable?
>
> Interestingly, do_test_single() in test_verifier.c already assumes
> that this is the intended use of data_size_out, and sets it to the
> output buffer size.
>
> Lorenz Bauer (3):
> bpf: respect size hint to BPF_PROG_TEST_RUN if present
> libbpf: require size hint in bpf_prog_test_run
> selftests: add a test for bpf_prog_test_run output size
>
> net/bpf/test_run.c | 9 ++++-
> tools/lib/bpf/bpf.c | 4 ++-
> tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
> 3 files changed, 55 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-18 7:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <CALCETrWdpoDkbZjkucKL91GWpDPG9p=VqYrULade2pFDR7S=GQ@mail.gmail.com>
On Thu, Nov 01, 2018 at 10:53:40AM -0700, Andy Lutomirski wrote:
> Hi all-
>
> The people working on SGX enablement are grappling with a somewhat
> annoying issue: the x86 EENTER instruction is used from user code and
> can, as part of its normal-ish operation, raise an exception. It is
> also highly likely to be used from a library, and signal handling in
> libraries is unpleasant at best.
>
> There's been some discussion of adding a vDSO entry point to wrap
> EENTER and do something sensible with the exceptions, but I'm
> wondering if a more general mechanism would be helpful.
I haven't really followed all of this discussion because I've been busy
working on the patch set but for me all of these approaches look awfully
complicated.
I'll throw my own suggestion and apologize if this has been already
suggested and discarded: return-to-AEP.
My idea is to do just a small extension to SGX AEX handling. At the
moment hardware will RAX, RBX and RCX with ERESUME parameters. We can
fill extend this by filling other three spare registers with exception
information.
AEP handler can then do whatever it wants to do with this information
or just do ERESUME.
In some ways this dummied version of Sean's suggestion.
I think whatever the solution is it should be lightweight and this is
such solution. Why? Because exception handling could be then used to
implement other stuff than just error hadling like syscall wrapper
for the enclaves in nice and lean way.
/Jarkko
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-18 7:18 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <20181118071548.GA4795@linux.intel.com>
On Sun, Nov 18, 2018 at 09:15:48AM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 01, 2018 at 10:53:40AM -0700, Andy Lutomirski wrote:
> > Hi all-
> >
> > The people working on SGX enablement are grappling with a somewhat
> > annoying issue: the x86 EENTER instruction is used from user code and
> > can, as part of its normal-ish operation, raise an exception. It is
> > also highly likely to be used from a library, and signal handling in
> > libraries is unpleasant at best.
> >
> > There's been some discussion of adding a vDSO entry point to wrap
> > EENTER and do something sensible with the exceptions, but I'm
> > wondering if a more general mechanism would be helpful.
>
> I haven't really followed all of this discussion because I've been busy
> working on the patch set but for me all of these approaches look awfully
> complicated.
>
> I'll throw my own suggestion and apologize if this has been already
> suggested and discarded: return-to-AEP.
>
> My idea is to do just a small extension to SGX AEX handling. At the
> moment hardware will RAX, RBX and RCX with ERESUME parameters. We can
> fill extend this by filling other three spare registers with exception
s/fill extend/extend/
/Jarkko
^ permalink raw reply
* [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 11:17 UTC (permalink / raw)
To: ebiederm, linux-kernel
Cc: serge, jannh, luto, akpm, oleg, cyphar, viro, linux-fsdevel,
linux-api, dancol, timmurray, Christian Brauner, Kees Cook
With this patch an open() call on /proc/<pid> will give userspace a handle
to struct pid of the process associated with /proc/<pid>. This allows to
maintain a stable handle on a process.
I have been discussing various approaches extensively during technical
conferences this year culminating in a long argument with Eric at Linux
Plumbers. The general consensus was that having a handle on a process
will be something that is very simple and easy to maintain with the
option of being extensible via a more advanced api if the need arises. I
believe that this patch is the most simple, dumb, and therefore
maintainable solution.
The need for this has arisen in order to reliably kill a process without
running into issues of the pid being recycled as has been described in the
rejected patch [1]. To fulfill the need described in that patchset a new
ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
process via a file descriptor:
int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
close(fd);
Note, the stable handle will allow us to carefully extend this feature in
the future.
[1]: https://lkml.org/lkml/2018/10/30/118
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
fs/proc/base.c | 33 +++++++++++++++++++++++++++++++++
include/uapi/linux/procfd.h | 11 +++++++++++
2 files changed, 44 insertions(+)
create mode 100644 include/uapi/linux/procfd.h
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ce3465479447..dfde564a21eb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -88,6 +88,7 @@
#include <linux/fs_struct.h>
#include <linux/slab.h>
#include <linux/sched/autogroup.h>
+#include <linux/sched/signal.h>
#include <linux/sched/mm.h>
#include <linux/sched/coredump.h>
#include <linux/sched/debug.h>
@@ -95,6 +96,7 @@
#include <linux/flex_array.h>
#include <linux/posix-timers.h>
#include <trace/events/oom.h>
+#include <uapi/linux/procfd.h>
#include "internal.h"
#include "fd.h"
@@ -3032,10 +3034,41 @@ static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
tgid_base_stuff, ARRAY_SIZE(tgid_base_stuff));
}
+static int proc_tgid_open(struct inode *inode, struct file *file)
+{
+ /* grab reference to struct pid */
+ file->private_data = get_pid(proc_pid(inode));
+ return 0;
+}
+
+static int proc_tgid_release(struct inode *inode, struct file *file)
+{
+ struct pid *pid = file->private_data;
+ /* drop reference to struct pid */
+ put_pid(pid);
+ return 0;
+}
+
+static long proc_tgid_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct pid *pid = file->private_data;
+
+ switch (cmd) {
+ case PROC_FD_SIGNAL:
+ return kill_pid(pid, arg, 1);
+ default:
+ return -EINVAL;
+ }
+}
+
static const struct file_operations proc_tgid_base_operations = {
+ .open = proc_tgid_open,
.read = generic_read_dir,
.iterate_shared = proc_tgid_base_readdir,
.llseek = generic_file_llseek,
+ .release = proc_tgid_release,
+ .unlocked_ioctl = proc_tgid_ioctl,
};
static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
diff --git a/include/uapi/linux/procfd.h b/include/uapi/linux/procfd.h
new file mode 100644
index 000000000000..8e4c07a9f3a3
--- /dev/null
+++ b/include/uapi/linux/procfd.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __LINUX_PROCFD_H
+#define __LINUX_PROCFD_H
+
+#include <linux/ioctl.h>
+
+/* Returns a file descriptor that refers to a struct pid */
+#define PROC_FD_SIGNAL _IOW('p', 1, __s32)
+
+#endif /* __LINUX_PROCFD_H */
+
--
2.19.1
^ permalink raw reply related
* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-18 13:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <20181118071548.GA4795@linux.intel.com>
On Sun, Nov 18, 2018 at 09:15:48AM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 01, 2018 at 10:53:40AM -0700, Andy Lutomirski wrote:
> > Hi all-
> >
> > The people working on SGX enablement are grappling with a somewhat
> > annoying issue: the x86 EENTER instruction is used from user code and
> > can, as part of its normal-ish operation, raise an exception. It is
> > also highly likely to be used from a library, and signal handling in
> > libraries is unpleasant at best.
> >
> > There's been some discussion of adding a vDSO entry point to wrap
> > EENTER and do something sensible with the exceptions, but I'm
> > wondering if a more general mechanism would be helpful.
>
> I haven't really followed all of this discussion because I've been busy
> working on the patch set but for me all of these approaches look awfully
> complicated.
>
> I'll throw my own suggestion and apologize if this has been already
> suggested and discarded: return-to-AEP.
>
> My idea is to do just a small extension to SGX AEX handling. At the
> moment hardware will RAX, RBX and RCX with ERESUME parameters. We can
> fill extend this by filling other three spare registers with exception
> information.
>
> AEP handler can then do whatever it wants to do with this information
> or just do ERESUME.
A correction here. In practice this will add a requirement to have a bit
more complicated AEP code (check the regs for exceptions) than before
and not just bytes for ENCLU.
e.g. AEP handler should be along the lines
1. #PF (or #UD or) happens. Kernel fills the registers when it cannot
handle the exception and returns back to user space i.e. to the
AEP handler.
2. Check the registers containing exception information. If they have
been filled, take whatever actions user space wants to take.
3. Otherwise, just ERESUME.
>From my point of view this is making the AEP parameter useful. Its
standard use is just weird (always point to a place just containing
ENCLU bytes, why the heck it even exists).
/Jarkko
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 13:59 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, linux-kernel, serge, Jann Horn, luto,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
linux-fsdevel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <20181118111751.6142-1-christian@brauner.io>
I had been led to believe that the proposal would be a comprehensive
process API, not an ioctl basically equivalent to my previous patch.
If you had a more comprehensive proposal, please just share it on LKML
instead of limiting the discussion to those able to attend these
various conferences. If there's some determined opposition to a
general new process API, this opposition needs a fair and full airing,
as not everyone can attend these conferences.
On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> With this patch an open() call on /proc/<pid> will give userspace a handle
> to struct pid of the process associated with /proc/<pid>. This allows to
> maintain a stable handle on a process.
> I have been discussing various approaches extensively during technical
> conferences this year culminating in a long argument with Eric at Linux
> Plumbers. The general consensus was that having a handle on a process
> will be something that is very simple and easy to maintain
ioctls are the opposite of "easy to maintain". Their
file-descriptor-specific behavior makes it difficult to use the things
safely. If you want to take this approach, please make a new system
call. An ioctl is just a system call with a very strange spelling and
unfortunate collision semantics.
> with the
> option of being extensible via a more advanced api if the need arises.
The need *has* arisen; see my exithand patch.
> I
> believe that this patch is the most simple, dumb, and therefore
> maintainable solution.
>
> The need for this has arisen in order to reliably kill a process without
> running into issues of the pid being recycled as has been described in the
> rejected patch [1].
That patch was not "rejected". It was tabled pending the more
comprehensive process API proposal that was supposed to have emerged.
This patch is just another variant of the sort of approach we
discussed on that patch's thread here. As I mentioned on that thread,
the right approach option is a new system call, not an ioctl.
To fulfill the need described in that patchset a new
> ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> process via a file descriptor:
>
> int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> close(fd);
>
> Note, the stable handle will allow us to carefully extend this feature in
> the future.
We still need the ability to synchronously wait on a process's death,
as in my patch set. I will be refreshing that patch set.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 15:38 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Lutomirski, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook
In-Reply-To: <CAKOZuet3s0WxxPgrtrNkq8YiOWhYAiK6yEio6PKVV9J_H4_TSA@mail.gmail.com>
On Sun, Nov 18, 2018 at 5:59 AM Daniel Colascione <dancol@google.com> wrote:
>
> I had been led to believe that the proposal would be a comprehensive
> process API, not an ioctl basically equivalent to my previous patch.
> If you had a more comprehensive proposal, please just share it on LKML
> instead of limiting the discussion to those able to attend these
> various conferences. If there's some determined opposition to a
> general new process API, this opposition needs a fair and full airing,
> as not everyone can attend these conferences.
>
> On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> > With this patch an open() call on /proc/<pid> will give userspace a handle
> > to struct pid of the process associated with /proc/<pid>. This allows to
> > maintain a stable handle on a process.
> > I have been discussing various approaches extensively during technical
> > conferences this year culminating in a long argument with Eric at Linux
> > Plumbers. The general consensus was that having a handle on a process
> > will be something that is very simple and easy to maintain
>
> ioctls are the opposite of "easy to maintain". Their
> file-descriptor-specific behavior makes it difficult to use the things
> safely. If you want to take this approach, please make a new system
> call. An ioctl is just a system call with a very strange spelling and
> unfortunate collision semantics.
>
> > with the
> > option of being extensible via a more advanced api if the need arises.
>
> The need *has* arisen; see my exithand patch.
>
> > I
> > believe that this patch is the most simple, dumb, and therefore
> > maintainable solution.
> >
> > The need for this has arisen in order to reliably kill a process without
> > running into issues of the pid being recycled as has been described in the
> > rejected patch [1].
>
> That patch was not "rejected". It was tabled pending the more
> comprehensive process API proposal that was supposed to have emerged.
> This patch is just another variant of the sort of approach we
> discussed on that patch's thread here. As I mentioned on that thread,
> the right approach option is a new system call, not an ioctl.
>
> To fulfill the need described in that patchset a new
> > ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> > process via a file descriptor:
> >
> > int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> > ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> > close(fd);
> >
> > Note, the stable handle will allow us to carefully extend this feature in
> > the future.
>
> We still need the ability to synchronously wait on a process's death,
> as in my patch set. I will be refreshing that patch set.
I fully agree that a more comprehensive, less expensive API for
managing processes would be nice. But I also think that this patch
(using the directory fd and ioctl) is better from a security
perspective than using a new file in /proc.
I have an old patch to make proc directory fds pollable:
https://lore.kernel.org/patchwork/patch/345098/
That patch plus the one in this thread might make a nice addition to
the kernel even if we expect something much better to come along
later.
This patch that uses ioctl
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 15:53 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <CALCETrUd2Y2MmO8Qx8aUvQDOTNiyj35Y7bctPBZe0p2i1EUiLw@mail.gmail.com>
On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> I fully agree that a more comprehensive, less expensive API for
> managing processes would be nice. But I also think that this patch
> (using the directory fd and ioctl) is better from a security
> perspective than using a new file in /proc.
That's an assertion, not an argument. And I'm not opposed to an
operation on the directory FD, now that it's clear Linus has banned
"write(2)-as-a-command" APIs. I just insist that we implement the API
with a system call instead of a less-reliable ioctl due to the
inherent namespace collision issues in ioctl command names.
> I have an old patch to make proc directory fds pollable:
>
> https://lore.kernel.org/patchwork/patch/345098/
>
> That patch plus the one in this thread might make a nice addition to
> the kernel even if we expect something much better to come along
> later.
I've always commented on that patch. You never addressed my technical
objections. Why are you bringing up this patch again as if that
discussion had never happened? To review, that patch has various race
conditions, and even if it were technically correct, it'd be an abuse
of directory objects (in what other circumstance do we poll
directories?) and not logically generalizable to a model in which we
expose process exit status via the exit-monitoring API.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 16:03 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, linux-kernel, Serge E. Hallyn, Jann Horn,
Andy Lutomirski, Andrew Morton, Oleg Nesterov, Aleksa Sarai,
Al Viro, Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <20181118111751.6142-1-christian@brauner.io>
On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> +static int proc_tgid_open(struct inode *inode, struct file *file)
> +{
> + /* grab reference to struct pid */
> + file->private_data = get_pid(proc_pid(inode));
Why grab another reference to the struct pid when the inode already has one?
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 16:17 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Lutomirski, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook
In-Reply-To: <CAKOZuev1JUGFWuwsKqS6rXcFMqpCHT1VAG2kwB4O=FHo6DAFiQ@mail.gmail.com>
On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > I fully agree that a more comprehensive, less expensive API for
> > managing processes would be nice. But I also think that this patch
> > (using the directory fd and ioctl) is better from a security
> > perspective than using a new file in /proc.
>
> That's an assertion, not an argument. And I'm not opposed to an
> operation on the directory FD, now that it's clear Linus has banned
> "write(2)-as-a-command" APIs. I just insist that we implement the API
> with a system call instead of a less-reliable ioctl due to the
> inherent namespace collision issues in ioctl command names.
Linus banned it because of bugs iike the ones in the patch.
>
> > I have an old patch to make proc directory fds pollable:
> >
> > https://lore.kernel.org/patchwork/patch/345098/
> >
> > That patch plus the one in this thread might make a nice addition to
> > the kernel even if we expect something much better to come along
> > later.
>
> I've always commented on that patch. You never addressed my technical
> objections. Why are you bringing up this patch again as if that
> discussion had never happened? To review, that patch has various race
> conditions
I don't think I ever saw that review.
> and even if it were technically correct, it'd be an abuse
> of directory objects (in what other circumstance do we poll
> directories?) and not logically generalizable to a model in which we
> expose process exit status via the exit-monitoring API.
I agree it's weird. It might be better to have /proc/PID/exit_status
and make *that* pollable.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 16:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <CALCETrVLP_mudJTW6EQpRr5GZ7kfuGci+QCT1uPrOVDTWcod-A@mail.gmail.com>
On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>
>> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> > I fully agree that a more comprehensive, less expensive API for
>> > managing processes would be nice. But I also think that this patch
>> > (using the directory fd and ioctl) is better from a security
>> > perspective than using a new file in /proc.
>>
>> That's an assertion, not an argument. And I'm not opposed to an
>> operation on the directory FD, now that it's clear Linus has banned
>> "write(2)-as-a-command" APIs. I just insist that we implement the API
>> with a system call instead of a less-reliable ioctl due to the
>> inherent namespace collision issues in ioctl command names.
>
> Linus banned it because of bugs iike the ones in the patch.
Maybe: he didn't provide a reason. What's your point?
>> > I have an old patch to make proc directory fds pollable:
>> >
>> > https://lore.kernel.org/patchwork/patch/345098/
>> >
>> > That patch plus the one in this thread might make a nice addition to
>> > the kernel even if we expect something much better to come along
>> > later.
>>
>> I've always commented on that patch. You never addressed my technical
>> objections. Why are you bringing up this patch again as if that
>> discussion had never happened? To review, that patch has various race
>> conditions
>
> I don't think I ever saw that review.
https://lore.kernel.org/lkml/CAKOZuevXXqwJepmLNUtrU=f8jtdgdKAzUAnAA2+KVcWoMxMyFg@mail.gmail.com/
>> and even if it were technically correct, it'd be an abuse
>> of directory objects (in what other circumstance do we poll
>> directories?) and not logically generalizable to a model in which we
>> expose process exit status via the exit-monitoring API.
>
> I agree it's weird. It might be better to have /proc/PID/exit_status
> and make *that* pollable.
That's exactly what my patch provides. I feel like we're rehashing the
previous discussion.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Randy Dunlap @ 2018-11-18 16:33 UTC (permalink / raw)
To: Andy Lutomirski, Daniel Colascione
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CALCETrVLP_mudJTW6EQpRr5GZ7kfuGci+QCT1uPrOVDTWcod-A@mail.gmail.com>
On 11/18/18 8:17 AM, Andy Lutomirski wrote:
> On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>
>> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> I fully agree that a more comprehensive, less expensive API for
>>> managing processes would be nice. But I also think that this patch
>>> (using the directory fd and ioctl) is better from a security
>>> perspective than using a new file in /proc.
>>
>> That's an assertion, not an argument. And I'm not opposed to an
>> operation on the directory FD, now that it's clear Linus has banned
>> "write(2)-as-a-command" APIs. I just insist that we implement the API
>> with a system call instead of a less-reliable ioctl due to the
>> inherent namespace collision issues in ioctl command names.
>
> Linus banned it because of bugs iike the ones in the patch.
>
>>
>>> I have an old patch to make proc directory fds pollable:
>>>
>>> https://lore.kernel.org/patchwork/patch/345098/
>>>
>>> That patch plus the one in this thread might make a nice addition to
>>> the kernel even if we expect something much better to come along
>>> later.
>>
>> I've always commented on that patch. You never addressed my technical
>> objections. Why are you bringing up this patch again as if that
>> discussion had never happened? To review, that patch has various race
>> conditions
>
> I don't think I ever saw that review.
>
>> and even if it were technically correct, it'd be an abuse
>> of directory objects (in what other circumstance do we poll
>> directories?) and not logically generalizable to a model in which we
>> expose process exit status via the exit-monitoring API.
>
> I agree it's weird. It might be better to have /proc/PID/exit_status
> and make *that* pollable.
>
If there is a new exit_status file, it could even be more than
8 bits of exit status:
See https://lore.kernel.org/lkml/alpine.LSU.2.20.1507091257010.9602@nerf40.vanv.qr/T/#u
and http://austingroupbugs.net/view.php?id=594#c1317
--
~Randy
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 16:48 UTC (permalink / raw)
To: Randy Dunlap
Cc: Andy Lutomirski, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, Jan Engelhardt
In-Reply-To: <a7f50692-667c-4efe-a2d0-fa324eebb90b@infradead.org>
On Sun, Nov 18, 2018 at 8:33 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 11/18/18 8:17 AM, Andy Lutomirski wrote:
>> On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>>
>>> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>>> I fully agree that a more comprehensive, less expensive API for
>>>> managing processes would be nice. But I also think that this patch
>>>> (using the directory fd and ioctl) is better from a security
>>>> perspective than using a new file in /proc.
>>>
>>> That's an assertion, not an argument. And I'm not opposed to an
>>> operation on the directory FD, now that it's clear Linus has banned
>>> "write(2)-as-a-command" APIs. I just insist that we implement the API
>>> with a system call instead of a less-reliable ioctl due to the
>>> inherent namespace collision issues in ioctl command names.
>>
>> Linus banned it because of bugs iike the ones in the patch.
>>
>>>
>>>> I have an old patch to make proc directory fds pollable:
>>>>
>>>> https://lore.kernel.org/patchwork/patch/345098/
>>>>
>>>> That patch plus the one in this thread might make a nice addition to
>>>> the kernel even if we expect something much better to come along
>>>> later.
>>>
>>> I've always commented on that patch. You never addressed my technical
>>> objections. Why are you bringing up this patch again as if that
>>> discussion had never happened? To review, that patch has various race
>>> conditions
>>
>> I don't think I ever saw that review.
>>
>>> and even if it were technically correct, it'd be an abuse
>>> of directory objects (in what other circumstance do we poll
>>> directories?) and not logically generalizable to a model in which we
>>> expose process exit status via the exit-monitoring API.
>>
>> I agree it's weird. It might be better to have /proc/PID/exit_status
>> and make *that* pollable.
>>
>
> If there is a new exit_status file, it could even be more than
> 8 bits of exit status:
>
> See https://lore.kernel.org/lkml/alpine.LSU.2.20.1507091257010.9602@nerf40.vanv.qr/T/#u
> and http://austingroupbugs.net/view.php?id=594#c1317
First of all, as I discussed in [1], we need to first figure out *who*
should have access to the process exit information. FreeBSD appears to
make it public without disaster, and if we make exit status public, a
lot of problems just disappear.
[1] https://lore.kernel.org/lkml/CAKOZueussVwABQaC+O9fW+MZayccvttKQZfWg0hh-cZ+1ykXig@mail.gmail.com/
Providing more than eight bits of status information is a separate
discussion. I don't think it's necessary, since it breaks assumptions
people make today without really adding much in the way of new
capabilities. If we want to let processes die while leaving behind
more information, let's let them leave behind an arbitrary-sized
string or something instead of repeating the mistake of an integral
exit status, but with more bits.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:09 UTC (permalink / raw)
To: Daniel Colascione
Cc: Randy Dunlap, Andrew Lutomirski, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZueutLc8d0Fe+7dNWiZKnALhTSST8+kCnOrL+OmB6coUmtA@mail.gmail.com>
On Sun, Nov 18, 2018 at 8:49 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Sun, Nov 18, 2018 at 8:33 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 11/18/18 8:17 AM, Andy Lutomirski wrote:
> >> On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
> >>>
> >>> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >>>> I fully agree that a more comprehensive, less expensive API for
> >>>> managing processes would be nice. But I also think that this patch
> >>>> (using the directory fd and ioctl) is better from a security
> >>>> perspective than using a new file in /proc.
> >>>
> >>> That's an assertion, not an argument. And I'm not opposed to an
> >>> operation on the directory FD, now that it's clear Linus has banned
> >>> "write(2)-as-a-command" APIs. I just insist that we implement the API
> >>> with a system call instead of a less-reliable ioctl due to the
> >>> inherent namespace collision issues in ioctl command names.
> >>
> >> Linus banned it because of bugs iike the ones in the patch.
> >>
> >>>
> >>>> I have an old patch to make proc directory fds pollable:
> >>>>
> >>>> https://lore.kernel.org/patchwork/patch/345098/
> >>>>
> >>>> That patch plus the one in this thread might make a nice addition to
> >>>> the kernel even if we expect something much better to come along
> >>>> later.
> >>>
> >>> I've always commented on that patch. You never addressed my technical
> >>> objections. Why are you bringing up this patch again as if that
> >>> discussion had never happened? To review, that patch has various race
> >>> conditions
> >>
> >> I don't think I ever saw that review.
> >>
> >>> and even if it were technically correct, it'd be an abuse
> >>> of directory objects (in what other circumstance do we poll
> >>> directories?) and not logically generalizable to a model in which we
> >>> expose process exit status via the exit-monitoring API.
> >>
> >> I agree it's weird. It might be better to have /proc/PID/exit_status
> >> and make *that* pollable.
> >>
> >
> > If there is a new exit_status file, it could even be more than
> > 8 bits of exit status:
> >
> > See https://lore.kernel.org/lkml/alpine.LSU.2.20.1507091257010.9602@nerf40.vanv.qr/T/#u
> > and http://austingroupbugs.net/view.php?id=594#c1317
>
> First of all, as I discussed in [1], we need to first figure out *who*
> should have access to the process exit information. FreeBSD appears to
> make it public without disaster, and if we make exit status public, a
> lot of problems just disappear.
I kind of want to go in the other direction of making a lot of process
information (especially cmdline) less publicly accessible.
In general, any kind of API where a process has an fd is tricky to do
right on UNIXy systems because of SUID, SGID, and LSM transition
rules. Windows has an easy time of it because it's always safe for a
parent process to introspect the child. (Well, almost, because
Windows gained their privilege elevation stuff. I'm not saying we
shouldn't do it -- I'm just saying that it's nontrivial.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:13 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Lutomirski, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook
In-Reply-To: <CAKOZuetuuxW+yFnyaqS5kW6gkVMstLa8D-7SgYtrLBUQB+sgDg@mail.gmail.com>
On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
> >>
> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> > I fully agree that a more comprehensive, less expensive API for
> >> > managing processes would be nice. But I also think that this patch
> >> > (using the directory fd and ioctl) is better from a security
> >> > perspective than using a new file in /proc.
> >>
> >> That's an assertion, not an argument. And I'm not opposed to an
> >> operation on the directory FD, now that it's clear Linus has banned
> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
> >> with a system call instead of a less-reliable ioctl due to the
> >> inherent namespace collision issues in ioctl command names.
> >
> > Linus banned it because of bugs iike the ones in the patch.
>
> Maybe: he didn't provide a reason. What's your point?
My point is that an API that involves a file like /proc/PID/kill is
very tricky to get right. Here are some considerations:
1. The .write handler for the file must not behave differently
depending on current. So we can't check the caller's creds. (There
are legacy things that are generally buggy that look at current's
creds in write handlers. They're legacy, and they're almost
universally at least a little bit buggy, and many have been
exploitable.)
2. Even if we have ioctl() or a new syscall on /proc/PID/kill, we at
least have to define whether *opening* kill checks any credentials.
It probably shouldn't, since I don't see what the semantics should be.
3. The current Linux kill_pid stuff doesn't take a cred parameter, so
it's not so easy to check f_cred even if we wanted to.
So the simplest implementation using /proc/PID/kill would be for .open
to do essentially nothing and for ioctl or a new syscall to check
credentials as usual. But this seems to have no technical advantage
over just using /proc/PID itself, and it's a good deal slower, as it
requires an open/close cycle.
Now if we had an ioctlat() API, maybe it would make sense. But we
don't, and I think it would be a bit crazy to add one.
--Andy
^ 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