| Message ID | 20200928090805.23343-5-lmb@cloudflare.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sockmap copying | expand |
On Mon, Sep 28, 2020 at 10:08:05AM +0100, Lorenz Bauer wrote: [ ... ] > SEC("iter/sockmap") > -int count_elems(struct bpf_iter__sockmap *ctx) > +int copy(struct bpf_iter__sockmap *ctx) > { > struct sock *sk = ctx->sk; > __u32 tmp, *key = ctx->key; > int ret; > > - if (key) > - elems++; > + if (!key) > + return 0; > > - if (sk) > + elems++; > + > + /* We need a temporary buffer on the stack, since the verifier doesn't > + * let us use the pointer from the context as an argument to the helper. Is it something that can be improved later? others LGTM.
On Tue, 29 Sep 2020 at 07:06, Martin KaFai Lau <kafai@fb.com> wrote: ... > > + /* We need a temporary buffer on the stack, since the verifier doesn't > > + * let us use the pointer from the context as an argument to the helper. > Is it something that can be improved later? > > others LGTM. Yeah, I think so. We'd need to do something similar to your sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the pointer is read only makes it a bit more difficult I think. After that, a user could just plug the key into map_update_elem directly. Alternatively, allow specialising map_ops per context.
On Tue, Sep 29, 2020 at 10:21:05AM +0100, Lorenz Bauer wrote: > On Tue, 29 Sep 2020 at 07:06, Martin KaFai Lau <kafai@fb.com> wrote: > > ... > > > > + /* We need a temporary buffer on the stack, since the verifier doesn't > > > + * let us use the pointer from the context as an argument to the helper. > > Is it something that can be improved later? > > > > others LGTM. > > Yeah, I think so. We'd need to do something similar to your > sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the > pointer is read only makes it a bit more difficult I think. After I thought the key arg should be used as read-only in the map's helper. or there is map type's helper that modifies the key? > that, a user could just plug the key into map_update_elem directly. > Alternatively, allow specialising map_ops per context.
On Tue, 29 Sep 2020 at 18:23, Martin KaFai Lau <kafai@fb.com> wrote: ... > > Yeah, I think so. We'd need to do something similar to your > > sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the > > pointer is read only makes it a bit more difficult I think. After > I thought the key arg should be used as read-only in the map's helper. > or there is map type's helper that modifies the key? I don't know, that's what I meant by more difficult. If map keys are always read-only like you say this would be straight forward to do (famous last words).
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c index e8a4bfb4d9f4..854a508e81ce 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c @@ -194,7 +194,7 @@ static void test_sockmap_invalid_update(void) test_sockmap_invalid_update__destroy(skel); } -static void test_sockmap_iter(enum bpf_map_type map_type) +static void test_sockmap_copy(enum bpf_map_type map_type) { DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts); int err, len, src_fd, iter_fd, duration = 0; @@ -242,7 +242,7 @@ static void test_sockmap_iter(enum bpf_map_type map_type) linfo.map.map_fd = src_fd; opts.link_info = &linfo; opts.link_info_len = sizeof(linfo); - link = bpf_program__attach_iter(skel->progs.count_elems, &opts); + link = bpf_program__attach_iter(skel->progs.copy, &opts); if (CHECK(IS_ERR(link), "attach_iter", "attach_iter failed\n")) goto out; @@ -265,6 +265,8 @@ static void test_sockmap_iter(enum bpf_map_type map_type) skel->bss->socks, num_sockets)) goto close_iter; + compare_cookies(src, skel->maps.dst); + close_iter: close(iter_fd); free_link: @@ -294,8 +296,8 @@ void test_sockmap_basic(void) test_sockmap_update(BPF_MAP_TYPE_SOCKHASH); if (test__start_subtest("sockmap update in unsafe context")) test_sockmap_invalid_update(); - if (test__start_subtest("sockmap iter")) - test_sockmap_iter(BPF_MAP_TYPE_SOCKMAP); - if (test__start_subtest("sockhash iter")) - test_sockmap_iter(BPF_MAP_TYPE_SOCKHASH); + if (test__start_subtest("sockmap copy")) + test_sockmap_copy(BPF_MAP_TYPE_SOCKMAP); + if (test__start_subtest("sockhash copy")) + test_sockmap_copy(BPF_MAP_TYPE_SOCKHASH); } diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c index 1af7555f6057..f3af0e30cead 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c @@ -22,21 +22,38 @@ struct { __type(value, __u64); } sockhash SEC(".maps"); +struct { + __uint(type, BPF_MAP_TYPE_SOCKHASH); + __uint(max_entries, 64); + __type(key, __u32); + __type(value, __u64); +} dst SEC(".maps"); + __u32 elems = 0; __u32 socks = 0; SEC("iter/sockmap") -int count_elems(struct bpf_iter__sockmap *ctx) +int copy(struct bpf_iter__sockmap *ctx) { struct sock *sk = ctx->sk; __u32 tmp, *key = ctx->key; int ret; - if (key) - elems++; + if (!key) + return 0; - if (sk) + elems++; + + /* We need a temporary buffer on the stack, since the verifier doesn't + * let us use the pointer from the context as an argument to the helper. + */ + tmp = *key; + + if (sk) { socks++; + return bpf_map_update_elem(&dst, &tmp, sk, 0) != 0; + } - return 0; + ret = bpf_map_delete_elem(&dst, &tmp); + return ret && ret != -ENOENT; }
Since we can now call map_update_elem(sockmap) from bpf_iter context it's possible to copy a sockmap or sockhash in the kernel. Add a selftest which exercises this. Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- .../selftests/bpf/prog_tests/sockmap_basic.c | 14 +++++----- .../selftests/bpf/progs/bpf_iter_sockmap.c | 27 +++++++++++++++---- 2 files changed, 30 insertions(+), 11 deletions(-)