diff mbox series

[bpf-next,v2,4/4] selftest: bpf: Test copying a sockmap and sockhash

Message ID 20200928090805.23343-5-lmb@cloudflare.com
State Superseded
Headers show
Series Sockmap copying | expand

Commit Message

Lorenz Bauer Sept. 28, 2020, 9:08 a.m. UTC
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(-)

Comments

Martin KaFai Lau Sept. 29, 2020, 6:06 a.m. UTC | #1
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.
Lorenz Bauer Sept. 29, 2020, 9:21 a.m. UTC | #2
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.
Martin KaFai Lau Sept. 29, 2020, 5:23 p.m. UTC | #3
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.
Lorenz Bauer Sept. 30, 2020, 9:37 a.m. UTC | #4
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 mbox series

Patch

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;
 }