* 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: extending wait4(2) or waitid(2) linux syscall
From: Arnd Bergmann @ 2018-11-16 15:17 UTC (permalink / raw)
To: Florian Weimer
Cc: H. Peter Anvin, Dmitry V. Levin, Albert ARIBAUD, GNU C Library,
Linux API
In-Reply-To: <87sh01uxlh.fsf@oldenburg.str.redhat.com>
On Fri, Nov 16, 2018 at 2:27 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Arnd Bergmann:
>
> > 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?
>
> Or introduce something that covers FreeBSD's wait6 functionality as well
> (resource usage by subprocesses of the subprocess).
Ok, so combining that with the request for a signal mask, we'd want
something like
int wait9(idtype_t idtype, id_t id, siginfo_t *infop, int options,
/* waitid subset */
int *status, struct rusage64 *self, struct rusage64
*children, /* wait4, wait6, time64 */
const sigset_t *sigmask, size_t sigsetsize);
/* suggested by Dmitry */
which I think could over all use cases mentioned in this thread, but
already has nine arguments instead of the allowed six, so it can't
be the signature of the system call entry.
We can get that down to six arguments in the familiar ways:
- like Linux waitid, wait4-style status can be computed from infop
- like wait6, self and children can be in one pointer
- like io_pgetevents, sigmask and sigsetsize can be an indirect struct
int waitid6(idtype_t idtype, id_t id, siginfo_t *infop, int options,
struct __wrusage64 *wrusage, struct __wait_sigset *sigset);
Alternatively, we could combine even more arguments into a single
structure if inputs, in various ways.
Arnd
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Dave Martin @ 2018-11-16 14:26 UTC (permalink / raw)
To: Florian Weimer
Cc: Dmitry V. Levin, Arnd Bergmann, Albert ARIBAUD, H. Peter Anvin,
GNU C Library, linux-api
In-Reply-To: <874lchrvha.fsf@oldenburg.str.redhat.com>
On Fri, Nov 16, 2018 at 02:40:49PM +0100, Florian Weimer wrote:
> * Dave Martin:
>
> > The traditional way would be install a handler for SIGCHLD and
> > do a sigsuspend()/pselect()/ppoll(). Then when the suspend() returns,
> > you pump the status of any children with wait*(..., WNOHANG).
> >
> > Does that not work for you for some reason?
>
> Changing signal handlers is not what a library should do because they
> are process-global (not even thread-specific).
True, though this is a problem with global resources too.
I guess I need to go and read the slides to understand the broader
context here. (Is strace a library?)
Cheers
---Dave
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Florian Weimer @ 2018-11-16 13:40 UTC (permalink / raw)
To: Dave Martin
Cc: Dmitry V. Levin, Arnd Bergmann, Albert ARIBAUD, H. Peter Anvin,
GNU C Library, linux-api
In-Reply-To: <20181116124238.GT3505@e103592.cambridge.arm.com>
* Dave Martin:
> The traditional way would be install a handler for SIGCHLD and
> do a sigsuspend()/pselect()/ppoll(). Then when the suspend() returns,
> you pump the status of any children with wait*(..., WNOHANG).
>
> Does that not work for you for some reason?
Changing signal handlers is not what a library should do because they
are process-global (not even thread-specific).
Thanks,
Florian
^ permalink raw reply
* [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size
From: Lorenz Bauer @ 2018-11-16 12:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
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;
+ }
+
+ 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 related
* [PATCH 2/3] libbpf: require size hint in bpf_prog_test_run
From: Lorenz Bauer @ 2018-11-16 12:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
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;
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 related
* [PATCH 1/3] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Lorenz Bauer @ 2018-11-16 12:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
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;
--
2.17.1
^ permalink raw reply related
* [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface
From: Lorenz Bauer @ 2018-11-16 12:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, Lorenz Bauer
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.
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: extending wait4(2) or waitid(2) linux syscall
From: Dave Martin @ 2018-11-16 12:42 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Arnd Bergmann, Albert ARIBAUD, H. Peter Anvin, GNU C Library,
linux-api
In-Reply-To: <20181115153008.GC2171@altlinux.org>
On Thu, Nov 15, 2018 at 06:30:11PM +0300, Dmitry V. Levin 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:
> [...]
> > > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign?rev=146
> > > Is there any rationale for marking wait4 as an obsolete API?
> >
> > In the *kernel* syscall API, wait4(2) is obsoleted by waitid(2), which is
> > a strict superset of its functionality.
> >
> > In the libc API, this is different, as wait4() does not have a replacement
> > that is exposed to user space directly. I expect glibc to implement
> > wait4() on top of the kernel's waitid().
> >
> > There has not been a final decision on which variant of waitid() that would
> > be. The easiest option would be to not change it at all: new architectures
> > (rv32, csky, nanomips/p32, ...) would keep exposing the traditional
> > waitid() in Linux, with its 32-bit time_t based rusage structure, but drop the
> > wait4(). glibc then has to convert between the kernel's rusage and the
> > user space rusage indefinitely.
> >
> > Alternatively, we can create a new version like waitid2() that uses
> > 64-bit time_t in some form, either the exact same rusage that we
> > use on 64-bit architectures and x32, or using a new set of arguments
> > to include further improvements.
>
> In strace, we have two use cases that require an extended version
> of wait4(2) or waitid(2) syscall. From your response I understand that
> you'd recommend extending waitid(2) rather than wait4(2), is it correct?
>
> These two use cases were mentioned in my talk yesterday at LPC 2018,
> here is a brief summary.
>
> 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).
>
> 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.
I didn't attend LPC so apologies if I'm missing some background here,
but:
The traditional way would be install a handler for SIGCHLD and
do a sigsuspend()/pselect()/ppoll(). Then when the suspend() returns,
you pump the status of any children with wait*(..., WNOHANG).
Does that not work for you for some reason?
Adding more and more "wait for some stuff" syscalls for every possible
definition of "some stuff" is a slippery slope... IMHO we have way too
many of those already.
One way to improve struct rusage would be to add a new wait flag (say,
WFANCY_RUSAGE) to the existing wait calls to indicate use of a different
struct in place of struct rusage. It's not clear to me why a brand new
syscall is essential, though obviously that's an option.
Cheers
---Dave
^ permalink raw reply
* [PATCH v3 4/4] tools: add selftest for BPF_F_ZERO_SEED
From: Lorenz Bauer @ 2018-11-16 11:41 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, songliubraving, Lorenz Bauer
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
Check that iterating two separate hash maps produces the same
order of keys if BPF_F_ZERO_SEED is used.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
1 file changed, 57 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 4db2116e52be..9f0a5b16a246 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -258,23 +258,35 @@ static void test_hashmap_percpu(int task, void *data)
close(fd);
}
+static int helper_fill_hashmap(int max_entries)
+{
+ int i, fd, ret;
+ long long key, value;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+ max_entries, map_flags);
+ CHECK(fd < 0,
+ "failed to create hashmap",
+ "err: %s, flags: 0x%x\n", strerror(errno), map_flags);
+
+ for (i = 0; i < max_entries; i++) {
+ key = i; value = key;
+ ret = bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST);
+ CHECK(ret != 0,
+ "can't update hashmap",
+ "err: %s\n", strerror(ret));
+ }
+
+ return fd;
+}
+
static void test_hashmap_walk(int task, void *data)
{
int fd, i, max_entries = 1000;
long long key, value, next_key;
bool next_key_valid = true;
- fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
- max_entries, map_flags);
- if (fd < 0) {
- printf("Failed to create hashmap '%s'!\n", strerror(errno));
- exit(1);
- }
-
- for (i = 0; i < max_entries; i++) {
- key = i; value = key;
- assert(bpf_map_update_elem(fd, &key, &value, BPF_NOEXIST) == 0);
- }
+ fd = helper_fill_hashmap(max_entries);
for (i = 0; bpf_map_get_next_key(fd, !i ? NULL : &key,
&next_key) == 0; i++) {
@@ -306,6 +318,39 @@ static void test_hashmap_walk(int task, void *data)
close(fd);
}
+static void test_hashmap_zero_seed(void)
+{
+ int i, first, second, old_flags;
+ long long key, next_first, next_second;
+
+ old_flags = map_flags;
+ map_flags |= BPF_F_ZERO_SEED;
+
+ first = helper_fill_hashmap(3);
+ second = helper_fill_hashmap(3);
+
+ for (i = 0; ; i++) {
+ void *key_ptr = !i ? NULL : &key;
+
+ if (bpf_map_get_next_key(first, key_ptr, &next_first) != 0)
+ break;
+
+ CHECK(bpf_map_get_next_key(second, key_ptr, &next_second) != 0,
+ "next_key for second map must succeed",
+ "key_ptr: %p", key_ptr);
+ CHECK(next_first != next_second,
+ "keys must match",
+ "i: %d first: %lld second: %lld\n", i,
+ next_first, next_second);
+
+ key = next_first;
+ }
+
+ map_flags = old_flags;
+ close(first);
+ close(second);
+}
+
static void test_arraymap(int task, void *data)
{
int key, next_key, fd;
@@ -1534,6 +1579,7 @@ static void run_all_tests(void)
test_hashmap(0, NULL);
test_hashmap_percpu(0, NULL);
test_hashmap_walk(0, NULL);
+ test_hashmap_zero_seed();
test_arraymap(0, NULL);
test_arraymap_percpu(0, NULL);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 3/4] tools: sync linux/bpf.h
From: Lorenz Bauer @ 2018-11-16 11:41 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, songliubraving, Lorenz Bauer
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
Synchronize changes to linux/bpf.h from
* "bpf: allow zero-initializing hash map seed"
* "bpf: move BPF_F_QUERY_EFFECTIVE after map flags"
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 852dc17ab47a..05d95290b848 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -257,9 +257,6 @@ enum bpf_attach_type {
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
-/* flags for BPF_PROG_QUERY */
-#define BPF_F_QUERY_EFFECTIVE (1U << 0)
-
#define BPF_OBJ_NAME_LEN 16U
/* Flags for accessing BPF object */
@@ -269,6 +266,12 @@ enum bpf_attach_type {
/* Flag for stack_map, store build_id+offset instead of pointer */
#define BPF_F_STACK_BUILD_ID (1U << 5)
+/* Zero-initialize hash function seed. This should only be used for testing. */
+#define BPF_F_ZERO_SEED (1U << 6)
+
+/* flags for BPF_PROG_QUERY */
+#define BPF_F_QUERY_EFFECTIVE (1U << 0)
+
enum bpf_stack_build_id_status {
/* user space need an empty entry to identify end of a trace */
BPF_STACK_BUILD_ID_EMPTY = 0,
@@ -2201,6 +2204,8 @@ union bpf_attr {
* **CONFIG_NET** configuration option.
* Return
* Pointer to *struct bpf_sock*, or NULL in case of failure.
+ * For sockets with reuseport option, *struct bpf_sock*
+ * return is from reuse->socks[] using hash of the packet.
*
* struct bpf_sock *bpf_sk_lookup_udp(void *ctx, struct bpf_sock_tuple *tuple, u32 tuple_size, u32 netns, u64 flags)
* Description
@@ -2233,6 +2238,8 @@ union bpf_attr {
* **CONFIG_NET** configuration option.
* Return
* Pointer to *struct bpf_sock*, or NULL in case of failure.
+ * For sockets with reuseport option, *struct bpf_sock*
+ * return is from reuse->socks[] using hash of the packet.
*
* int bpf_sk_release(struct bpf_sock *sk)
* Description
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/4] bpf: move BPF_F_QUERY_EFFECTIVE after map flags
From: Lorenz Bauer @ 2018-11-16 11:41 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, songliubraving, Lorenz Bauer
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
BPF_F_QUERY_EFFECTIVE is in the middle of the flags valid
for BPF_MAP_CREATE. Move it to its own section to reduce confusion.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8c01b89a4cb4..05d95290b848 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -257,9 +257,6 @@ enum bpf_attach_type {
/* Specify numa node during map creation */
#define BPF_F_NUMA_NODE (1U << 2)
-/* flags for BPF_PROG_QUERY */
-#define BPF_F_QUERY_EFFECTIVE (1U << 0)
-
#define BPF_OBJ_NAME_LEN 16U
/* Flags for accessing BPF object */
@@ -272,6 +269,9 @@ enum bpf_attach_type {
/* Zero-initialize hash function seed. This should only be used for testing. */
#define BPF_F_ZERO_SEED (1U << 6)
+/* flags for BPF_PROG_QUERY */
+#define BPF_F_QUERY_EFFECTIVE (1U << 0)
+
enum bpf_stack_build_id_status {
/* user space need an empty entry to identify end of a trace */
BPF_STACK_BUILD_ID_EMPTY = 0,
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/4] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-11-16 11:41 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, songliubraving, Lorenz Bauer
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
Add a new flag BPF_F_ZERO_SEED, which forces a hash map
to initialize the seed to zero. This is useful when doing
performance analysis both on individual BPF programs, as
well as the kernel's hash table implementation.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 3 +++
kernel/bpf/hashtab.c | 13 +++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 47d606d744cc..8c01b89a4cb4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -269,6 +269,9 @@ enum bpf_attach_type {
/* Flag for stack_map, store build_id+offset instead of pointer */
#define BPF_F_STACK_BUILD_ID (1U << 5)
+/* Zero-initialize hash function seed. This should only be used for testing. */
+#define BPF_F_ZERO_SEED (1U << 6)
+
enum bpf_stack_build_id_status {
/* user space need an empty entry to identify end of a trace */
BPF_STACK_BUILD_ID_EMPTY = 0,
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 2c1790288138..4b7c76765d9d 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -23,7 +23,7 @@
#define HTAB_CREATE_FLAG_MASK \
(BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
- BPF_F_RDONLY | BPF_F_WRONLY)
+ BPF_F_RDONLY | BPF_F_WRONLY | BPF_F_ZERO_SEED)
struct bucket {
struct hlist_nulls_head head;
@@ -244,6 +244,7 @@ static int htab_map_alloc_check(union bpf_attr *attr)
*/
bool percpu_lru = (attr->map_flags & BPF_F_NO_COMMON_LRU);
bool prealloc = !(attr->map_flags & BPF_F_NO_PREALLOC);
+ bool zero_seed = (attr->map_flags & BPF_F_ZERO_SEED);
int numa_node = bpf_map_attr_numa_node(attr);
BUILD_BUG_ON(offsetof(struct htab_elem, htab) !=
@@ -257,6 +258,10 @@ static int htab_map_alloc_check(union bpf_attr *attr)
*/
return -EPERM;
+ if (zero_seed && !capable(CAP_SYS_ADMIN))
+ /* Guard against local DoS, and discourage production use. */
+ return -EPERM;
+
if (attr->map_flags & ~HTAB_CREATE_FLAG_MASK)
/* reserved bits should not be used */
return -EINVAL;
@@ -373,7 +378,11 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
if (!htab->buckets)
goto free_htab;
- htab->hashrnd = get_random_int();
+ if (htab->map.map_flags & BPF_F_ZERO_SEED)
+ htab->hashrnd = 0;
+ else
+ htab->hashrnd = get_random_int();
+
for (i = 0; i < htab->n_buckets; i++) {
INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
raw_spin_lock_init(&htab->buckets[i].lock);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/4] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-11-16 11:41 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, songliubraving, Lorenz Bauer
In-Reply-To: <20181001104509.24211-1-lmb@cloudflare.com>
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: extending wait4(2) or waitid(2) linux syscall
From: Florian Weimer @ 2018-11-16 10:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: H. Peter Anvin, Dmitry V. Levin, Albert ARIBAUD, GNU C Library,
Linux API
In-Reply-To: <CAK8P3a2bGYt1Z50eQsBv2pu_zd649SB1MjX-iWcZ-YnJ+Bhj1Q@mail.gmail.com>
* Arnd Bergmann:
> 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?
Or introduce something that covers FreeBSD's wait6 functionality as well
(resource usage by subprocesses of the subprocess).
Thanks,
Florian
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Arnd Bergmann @ 2018-11-16 7:14 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Dmitry V. Levin, Albert ARIBAUD, GNU C Library, Linux API
In-Reply-To: <68E78315-66C9-4CEE-B6A0-61485AFB314A@zytor.com>
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
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Arnd Bergmann @ 2018-11-16 7:12 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:
> [...]
> > > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign?rev=146
> > > Is there any rationale for marking wait4 as an obsolete API?
> >
> > In the *kernel* syscall API, wait4(2) is obsoleted by waitid(2), which is
> > a strict superset of its functionality.
> >
> > In the libc API, this is different, as wait4() does not have a replacement
> > that is exposed to user space directly. I expect glibc to implement
> > wait4() on top of the kernel's waitid().
> >
> > There has not been a final decision on which variant of waitid() that would
> > be. The easiest option would be to not change it at all: new architectures
> > (rv32, csky, nanomips/p32, ...) would keep exposing the traditional
> > waitid() in Linux, with its 32-bit time_t based rusage structure, but drop the
> > wait4(). glibc then has to convert between the kernel's rusage and the
> > user space rusage indefinitely.
> >
> > Alternatively, we can create a new version like waitid2() that uses
> > 64-bit time_t in some form, either the exact same rusage that we
> > use on 64-bit architectures and x32, or using a new set of arguments
> > to include further improvements.
>
> In strace, we have two use cases that require an extended version
> of wait4(2) or waitid(2) syscall. From your response I understand that
> you'd recommend extending waitid(2) rather than wait4(2), is it correct?
Correct. It's already a superset, so a new waitid2(2) or wait5(2) should be
an extension of waitid(2) in order to provide backwards compatibility to the
other ones (along with wait() and waitpid()).
> These two use cases were mentioned in my talk yesterday at LPC 2018,
> here is a brief summary.
>
> 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).
Right, that indirection is not ideal, but I suspect it's better
than the alternatives.
> 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.
It definitely has to be a 64-bit based structure, the question is which one.
My preferred solution would be to interpret the timestamps as 'struct
__kernel_timespec', which has a 64-bit seconds and 64-bit nanoseconds.
I'd also use a structure that is the same between 32-bit and 64-bit
kernels here, using '__s64' members instead of '__kernel_long_t' or 'long'
for the rest. It is then up to the C library to convert it into whichever
structure they want to expose to user space for the normal wait4().
Arnd
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Carlos O'Donell @ 2018-11-15 21:00 UTC (permalink / raw)
To: Theodore Y. Ts'o, Joseph Myers, Daniel Colascione,
Szabolcs Nagy, Dave P Martin, nd, Florian Weimer,
Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka,
libc-alpha@sourceware.org
In-Reply-To: <20181115170807.GB20617@thunk.org>
On 11/15/18 12:08 PM, Theodore Y. Ts'o wrote:
> On Thu, Nov 15, 2018 at 04:29:43PM +0000, Joseph Myers wrote:
>> On Thu, 15 Nov 2018, Theodore Y. Ts'o wrote:
>>
>>> That's great. But is it or is it not true (either de jure or de
>>> facto) that "a single active glibc developer" can block a system call
>>> from being supported by glibc by objecting? And if not, under what is
>>> the process by resolving a conflict?
>>
>> We use a consensus-building process as described at
>> <https://sourceware.org/glibc/wiki/Consensus>.
>
> So can a single glibc developer can block Consensus?
Yes.
I think the comparison to the "liberum veto" is not a fair
comparison to the way the glibc community works :-)
(1) Community consensus.
Consensus need not imply unanimity.
Consensus is only from the set of important and concerned
interests. The community gets to decide that you're a troll
that does no real work, and can therefore ignore you.
Consensus is blocked only by sustained objection (not just
normal objections, which are recorded as part of the
development process e.g. "I don't like it, but I leave it
up to you to decide").
Therefore an involved glibc developer can lodge a sustained
objection, and block consensus.
(2) The GNU package maintainers for glibc.
There are 8 GNU package maintainers for glibc.
The package maintainers created the consensus process to
empower the community, but they can act as a final
review committee to move issues where there are two
reasonable but competing view points.
As Joseph points out we haven't ever used the GNU pakcage
maintainers to vote on a stuck issue, but I will arrange
it when the need arises. If you think we're at that point
with wrapper functions, just say so, but it doesn't seem
like it to me.
--
Cheers,
Carlos.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Carlos O'Donell @ 2018-11-15 20:34 UTC (permalink / raw)
To: Joseph Myers, Daniel Colascione
Cc: Szabolcs Nagy, Dave P Martin, nd, Florian Weimer,
Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka,
libc-alpha@sourceware.org
In-Reply-To: <alpine.DEB.2.21.1811141840140.16571@digraph.polyomino.org.uk>
On 11/14/18 1:47 PM, Joseph Myers wrote:
> On Wed, 14 Nov 2018, Daniel Colascione wrote:
>
>> A good demonstration of a new commitment to pragmatism would be
>> merging the trivial wrappers for gettid(2).
>
> I support the addition of gettid (for use with those syscalls that take
> tids, and with appropriate documentation explaining the properties of
> tids) - and, generally, wrappers for all non-obsolescent
> architecture-independent Linux kernel syscalls, including ones that are
> very Linux-specific, except maybe for a few interfaces fundamentally
> inconsistent with glibc managing TLS etc. - they are, at least, no worse
> as a source of APIs than all the old BSD / SVID interfaces we have from
> when those were used as sources of APIs.
I agree. Documentation is important.
--
Cheers,
Carlos.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Joseph Myers @ 2018-11-15 17:14 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Daniel Colascione, Szabolcs Nagy, Dave P Martin, nd,
Florian Weimer, Michael Kerrisk (man-pages), linux-kernel,
Joel Fernandes, Linux API, Willy Tarreau, Vlastimil Babka,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <20181115170807.GB20617@thunk.org>
On Thu, 15 Nov 2018, Theodore Y. Ts'o wrote:
> On Thu, Nov 15, 2018 at 04:29:43PM +0000, Joseph Myers wrote:
> > On Thu, 15 Nov 2018, Theodore Y. Ts'o wrote:
> >
> > > That's great. But is it or is it not true (either de jure or de
> > > facto) that "a single active glibc developer" can block a system call
> > > from being supported by glibc by objecting? And if not, under what is
> > > the process by resolving a conflict?
> >
> > We use a consensus-building process as described at
> > <https://sourceware.org/glibc/wiki/Consensus>.
>
> So can a single glibc developer can block Consensus?
If it's a sustained objection - it still works an awful lot better than
how things worked before 2011/12. (See my suggestion of having a process
involving a supermajority vote of the GNU maintainers for glibc in the
rare cases where a consensus cannot be reached - but those are rare enough
that actually agreeing a process for such cases has never been a
priority.)
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Theodore Y. Ts'o @ 2018-11-15 17:08 UTC (permalink / raw)
To: Joseph Myers
Cc: Daniel Colascione, Szabolcs Nagy, Dave P Martin, nd,
Florian Weimer, Michael Kerrisk (man-pages), linux-kernel,
Joel Fernandes, Linux API, Willy Tarreau, Vlastimil Babka,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <alpine.DEB.2.21.1811151628490.27351@digraph.polyomino.org.uk>
On Thu, Nov 15, 2018 at 04:29:43PM +0000, Joseph Myers wrote:
> On Thu, 15 Nov 2018, Theodore Y. Ts'o wrote:
>
> > That's great. But is it or is it not true (either de jure or de
> > facto) that "a single active glibc developer" can block a system call
> > from being supported by glibc by objecting? And if not, under what is
> > the process by resolving a conflict?
>
> We use a consensus-building process as described at
> <https://sourceware.org/glibc/wiki/Consensus>.
So can a single glibc developer can block Consensus?
I've chaired IETF working groups, where the standard was "Rough
Consensus and Running Code". Strict Consensus very easily ends up
leading to the Librem Veto which did not serve the Polish-Lithuanian
Commonwealth well in the 17th-18th centuries....
- Ted
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Joseph Myers @ 2018-11-15 16:29 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Daniel Colascione, Szabolcs Nagy, Dave P Martin, nd,
Florian Weimer, Michael Kerrisk (man-pages), linux-kernel,
Joel Fernandes, Linux API, Willy Tarreau, Vlastimil Babka,
Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <20181115053026.GA20617@thunk.org>
On Thu, 15 Nov 2018, Theodore Y. Ts'o wrote:
> That's great. But is it or is it not true (either de jure or de
> facto) that "a single active glibc developer" can block a system call
> from being supported by glibc by objecting? And if not, under what is
> the process by resolving a conflict?
We use a consensus-building process as described at
<https://sourceware.org/glibc/wiki/Consensus>.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: hpa @ 2018-11-15 15:37 UTC (permalink / raw)
To: Dmitry V. Levin, Arnd Bergmann; +Cc: Albert ARIBAUD, GNU C Library, linux-api
In-Reply-To: <20181115153008.GC2171@altlinux.org>
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:
>[...]
>> > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign?rev=146
>> > Is there any rationale for marking wait4 as an obsolete API?
>>
>> In the *kernel* syscall API, wait4(2) is obsoleted by waitid(2),
>which is
>> a strict superset of its functionality.
>>
>> In the libc API, this is different, as wait4() does not have a
>replacement
>> that is exposed to user space directly. I expect glibc to implement
>> wait4() on top of the kernel's waitid().
>>
>> There has not been a final decision on which variant of waitid() that
>would
>> be. The easiest option would be to not change it at all: new
>architectures
>> (rv32, csky, nanomips/p32, ...) would keep exposing the traditional
>> waitid() in Linux, with its 32-bit time_t based rusage structure, but
>drop the
>> wait4(). glibc then has to convert between the kernel's rusage and
>the
>> user space rusage indefinitely.
>>
>> Alternatively, we can create a new version like waitid2() that uses
>> 64-bit time_t in some form, either the exact same rusage that we
>> use on 64-bit architectures and x32, or using a new set of arguments
>> to include further improvements.
>
>In strace, we have two use cases that require an extended version
>of wait4(2) or waitid(2) syscall. From your response I understand that
>you'd recommend extending waitid(2) rather than wait4(2), is it
>correct?
>
>These two use cases were mentioned in my talk yesterday at LPC 2018,
>here is a brief summary.
>
>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).
>
>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.
--
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: Dmitry V. Levin @ 2018-11-15 15:30 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Albert ARIBAUD, H. Peter Anvin, GNU C Library, linux-api
In-Reply-To: <CAK8P3a0Gsqa8WTbALOUchRyEA7E2f3P1f=XQ8nD2xQaemfPpcQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]
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:
[...]
> > > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign?rev=146
> > Is there any rationale for marking wait4 as an obsolete API?
>
> In the *kernel* syscall API, wait4(2) is obsoleted by waitid(2), which is
> a strict superset of its functionality.
>
> In the libc API, this is different, as wait4() does not have a replacement
> that is exposed to user space directly. I expect glibc to implement
> wait4() on top of the kernel's waitid().
>
> There has not been a final decision on which variant of waitid() that would
> be. The easiest option would be to not change it at all: new architectures
> (rv32, csky, nanomips/p32, ...) would keep exposing the traditional
> waitid() in Linux, with its 32-bit time_t based rusage structure, but drop the
> wait4(). glibc then has to convert between the kernel's rusage and the
> user space rusage indefinitely.
>
> Alternatively, we can create a new version like waitid2() that uses
> 64-bit time_t in some form, either the exact same rusage that we
> use on 64-bit architectures and x32, or using a new set of arguments
> to include further improvements.
In strace, we have two use cases that require an extended version
of wait4(2) or waitid(2) syscall. From your response I understand that
you'd recommend extending waitid(2) rather than wait4(2), is it correct?
These two use cases were mentioned in my talk yesterday at LPC 2018,
here is a brief summary.
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).
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.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Dave Martin @ 2018-11-15 10:33 UTC (permalink / raw)
To: Florian Weimer
Cc: Andy Lutomirski, Daniel Colascione, Michael Kerrisk (man-pages),
linux-kernel, Joel Fernandes, Linux API, Willy Tarreau,
Vlastimil Babka, Carlos O'Donell, libc-alpha@sourceware.org
In-Reply-To: <877ehfdgzn.fsf@oldenburg.str.redhat.com>
On Wed, Nov 14, 2018 at 12:40:44PM +0100, Florian Weimer wrote:
> * Dave Martin:
>
> > Fair points, though this is rather what I meant by "sane essentials".
> > Because there are strict limits on what can be done in the vDSO, it may
> > be more bloat-resistant and more conservatively maintained.
> >
> > This might provide a way to push some dumb compatibility kludge code
> > that receives little ongoing maintenance outside the privilege wall,
> > whereas it has to sit in the kernel proper today.
> >
> > In theory we could opt to advertise new syscalls only via vDSO entry
> > points, and not maintain __NR_xxx values for them (which may or may
> > not upset ptrace users.) Anyway, I digress...
>
> Is the vDSO available across all architectures? (I don't think we use
> it on all architectures in glibc.)
It's probably not available on all arches.
> If not, a vDSO-based approach would merely lead to even more variance
> between architectures, which can't be a good thing.
That's a fair concern.
Channeling syscalls through the vDSO could allow for a uniform syscall
interface at the ELF linkage level, but only those arches that have a
vDSO. There may be other issues too.
Also, I don't say that we should definitely do this, just that it's
a possibility.
Cheers
---Dave
^ 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