diff mbox series

[bpf-next,5/5] bpf, testing: Add selftest to read/write sockaddr from user space

Message ID 19ce2c58465c5fab4c94f23450a8b8d5016a35bb.1572010897.git.daniel@iogearbox.net
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series Fix BPF probe memory helpers | expand

Commit Message

Daniel Borkmann Oct. 25, 2019, 4:37 p.m. UTC 
Tested on x86-64 and Ilya was also kind enough to give it a spin on
s390x, both passing with probe_user:OK there. The test is using the
newly added bpf_probe_read_user() to dump sockaddr from connect call
into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c

Comments

Andrii Nakryiko Oct. 25, 2019, 10:14 p.m. UTC | #1 
On Fri, Oct 25, 2019 at 1:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Tested on x86-64 and Ilya was also kind enough to give it a spin on
> s390x, both passing with probe_user:OK there. The test is using the
> newly added bpf_probe_read_user() to dump sockaddr from connect call
> into BPF map and overrides the user buffer via bpf_probe_write_user():
>
>   # ./test_progs
>   [...]
>   #17 pkt_md_access:OK
>   #18 probe_user:OK
>   #19 prog_run_xattr:OK
>   [...]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
>  .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
>  2 files changed, 113 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> new file mode 100644
> index 000000000000..e37761bda8a4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +
> +void test_probe_user(void)
> +{
> +#define kprobe_name "__sys_connect"
> +       const char *prog_name = "kprobe/" kprobe_name;
> +       const char *obj_file = "./test_probe_user.o";
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> +               .relaxed_maps = true,

do we need relaxed_maps in this case?

> +       );
> +       int err, results_map_fd, sock_fd, duration;
> +       struct sockaddr curr, orig, tmp;
> +       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
> +       struct bpf_link *kprobe_link = NULL;
> +       struct bpf_program *kprobe_prog;
> +       struct bpf_object *obj;
> +       static const int zero = 0;
> +

[...]

> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> +       __uint(max_entries, 1);
> +       __type(key, int);
> +       __type(value, struct sockaddr_in);
> +} results_map SEC(".maps");
> +
> +SEC("kprobe/__sys_connect")
> +int handle_sys_connect(struct pt_regs *ctx)
> +{
> +       void *ptr = (void *)PT_REGS_PARM2(ctx);
> +       struct sockaddr_in old, new;
> +       const int zero = 0;
> +
> +       bpf_probe_read_user(&old, sizeof(old), ptr);
> +       bpf_map_update_elem(&results_map, &zero, &old, 0);

could have used global data and read directly into it :)

> +       __builtin_memset(&new, 0xab, sizeof(new));
> +       bpf_probe_write_user(ptr, &new, sizeof(new));
> +
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.21.0
>
Daniel Borkmann Oct. 25, 2019, 10:38 p.m. UTC | #2 
On Fri, Oct 25, 2019 at 03:14:49PM -0700, Andrii Nakryiko wrote:
> On Fri, Oct 25, 2019 at 1:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > Tested on x86-64 and Ilya was also kind enough to give it a spin on
> > s390x, both passing with probe_user:OK there. The test is using the
> > newly added bpf_probe_read_user() to dump sockaddr from connect call
> > into BPF map and overrides the user buffer via bpf_probe_write_user():
> >
> >   # ./test_progs
> >   [...]
> >   #17 pkt_md_access:OK
> >   #18 probe_user:OK
> >   #19 prog_run_xattr:OK
> >   [...]
> >
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
> >  .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
> >  2 files changed, 113 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> > new file mode 100644
> > index 000000000000..e37761bda8a4
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <test_progs.h>
> > +
> > +void test_probe_user(void)
> > +{
> > +#define kprobe_name "__sys_connect"
> > +       const char *prog_name = "kprobe/" kprobe_name;
> > +       const char *obj_file = "./test_probe_user.o";
> > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > +               .relaxed_maps = true,
> 
> do we need relaxed_maps in this case?

Ah yeap, I'll remove. Test runs fine w/o it. Any particular reason you added it back in
928ca75e59d7 ("selftests/bpf: switch tests to new bpf_object__open_{file, mem}() APIs")?

> > +       );
> > +       int err, results_map_fd, sock_fd, duration;
> > +       struct sockaddr curr, orig, tmp;
> > +       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
> > +       struct bpf_link *kprobe_link = NULL;
> > +       struct bpf_program *kprobe_prog;
> > +       struct bpf_object *obj;
> > +       static const int zero = 0;
> > +
> 
> [...]
> 
> > +
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_ARRAY);
> > +       __uint(max_entries, 1);
> > +       __type(key, int);
> > +       __type(value, struct sockaddr_in);
> > +} results_map SEC(".maps");
> > +
> > +SEC("kprobe/__sys_connect")
> > +int handle_sys_connect(struct pt_regs *ctx)
> > +{
> > +       void *ptr = (void *)PT_REGS_PARM2(ctx);
> > +       struct sockaddr_in old, new;
> > +       const int zero = 0;
> > +
> > +       bpf_probe_read_user(&old, sizeof(old), ptr);
> > +       bpf_map_update_elem(&results_map, &zero, &old, 0);
> 
> could have used global data and read directly into it :)

Hehe, yeah sure, though that we have covered separately. :-) Wasn't planning to
bug Ilya once again to recompile everything on his s390x box.

> > +       __builtin_memset(&new, 0xab, sizeof(new));
> > +       bpf_probe_write_user(ptr, &new, sizeof(new));
> > +
> > +       return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > --
> > 2.21.0
> >
Andrii Nakryiko Oct. 25, 2019, 11:35 p.m. UTC | #3 
On Fri, Oct 25, 2019 at 4:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Fri, Oct 25, 2019 at 03:14:49PM -0700, Andrii Nakryiko wrote:
> > On Fri, Oct 25, 2019 at 1:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > Tested on x86-64 and Ilya was also kind enough to give it a spin on
> > > s390x, both passing with probe_user:OK there. The test is using the
> > > newly added bpf_probe_read_user() to dump sockaddr from connect call
> > > into BPF map and overrides the user buffer via bpf_probe_write_user():
> > >
> > >   # ./test_progs
> > >   [...]
> > >   #17 pkt_md_access:OK
> > >   #18 probe_user:OK
> > >   #19 prog_run_xattr:OK
> > >   [...]
> > >
> > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > > Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
> > >  .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
> > >  2 files changed, 113 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
> > >  create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> > > new file mode 100644
> > > index 000000000000..e37761bda8a4
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c
> > > @@ -0,0 +1,80 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <test_progs.h>
> > > +
> > > +void test_probe_user(void)
> > > +{
> > > +#define kprobe_name "__sys_connect"
> > > +       const char *prog_name = "kprobe/" kprobe_name;
> > > +       const char *obj_file = "./test_probe_user.o";
> > > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > > +               .relaxed_maps = true,
> >
> > do we need relaxed_maps in this case?
>
> Ah yeap, I'll remove. Test runs fine w/o it. Any particular reason you added it back in
> 928ca75e59d7 ("selftests/bpf: switch tests to new bpf_object__open_{file, mem}() APIs")?

Hmm, I'm not sure about those tests... probably just copy/pasted
something mechanically. We need .relaxed_maps for tests that add numa
fields, otherwise libbpf will reject them. I shouldn't have added
them.

>
> > > +       );
> > > +       int err, results_map_fd, sock_fd, duration;
> > > +       struct sockaddr curr, orig, tmp;
> > > +       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
> > > +       struct bpf_link *kprobe_link = NULL;
> > > +       struct bpf_program *kprobe_prog;
> > > +       struct bpf_object *obj;
> > > +       static const int zero = 0;
> > > +
> >
> > [...]
> >
> > > +
> > > +struct {
> > > +       __uint(type, BPF_MAP_TYPE_ARRAY);
> > > +       __uint(max_entries, 1);
> > > +       __type(key, int);
> > > +       __type(value, struct sockaddr_in);
> > > +} results_map SEC(".maps");
> > > +
> > > +SEC("kprobe/__sys_connect")
> > > +int handle_sys_connect(struct pt_regs *ctx)
> > > +{
> > > +       void *ptr = (void *)PT_REGS_PARM2(ctx);
> > > +       struct sockaddr_in old, new;
> > > +       const int zero = 0;
> > > +
> > > +       bpf_probe_read_user(&old, sizeof(old), ptr);
> > > +       bpf_map_update_elem(&results_map, &zero, &old, 0);
> >
> > could have used global data and read directly into it :)
>
> Hehe, yeah sure, though that we have covered separately. :-) Wasn't planning to
> bug Ilya once again to recompile everything on his s390x box.

Oh, it's not to test global data, it's because global data make BPF
side of tests much cleaner. But it's minor, feel free to ignore. Once
we have a good interface to global data from user-space, though, there
will be a bigger motivation to switch them to global data, because
both BPF and user side will be much more succinct.

>
> > > +       __builtin_memset(&new, 0xab, sizeof(new));
> > > +       bpf_probe_write_user(ptr, &new, sizeof(new));
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > --
> > > 2.21.0
> > >
Andrii Nakryiko Oct. 25, 2019, 11:36 p.m. UTC | #4 
On Fri, Oct 25, 2019 at 1:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Tested on x86-64 and Ilya was also kind enough to give it a spin on
> s390x, both passing with probe_user:OK there. The test is using the
> newly added bpf_probe_read_user() to dump sockaddr from connect call
> into BPF map and overrides the user buffer via bpf_probe_write_user():
>
>   # ./test_progs
>   [...]
>   #17 pkt_md_access:OK
>   #18 probe_user:OK
>   #19 prog_run_xattr:OK
>   [...]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
>  .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
>  2 files changed, 113 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c

[...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
new file mode 100644
index 000000000000..e37761bda8a4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c
@@ -0,0 +1,80 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_probe_user(void)
+{
+#define kprobe_name "__sys_connect"
+	const char *prog_name = "kprobe/" kprobe_name;
+	const char *obj_file = "./test_probe_user.o";
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+		.relaxed_maps = true,
+	);
+	int err, results_map_fd, sock_fd, duration;
+	struct sockaddr curr, orig, tmp;
+	struct sockaddr_in *in = (struct sockaddr_in *)&curr;
+	struct bpf_link *kprobe_link = NULL;
+	struct bpf_program *kprobe_prog;
+	struct bpf_object *obj;
+	static const int zero = 0;
+
+	obj = bpf_object__open_file(obj_file, &opts);
+	if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
+		return;
+
+	kprobe_prog = bpf_object__find_program_by_title(obj, prog_name);
+	if (CHECK(!kprobe_prog, "find_probe",
+		  "prog '%s' not found\n", prog_name))
+		goto cleanup;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto cleanup;
+
+	results_map_fd = bpf_find_map(__func__, obj, "results_map");
+	if (CHECK(results_map_fd < 0, "find_results_map",
+		  "err %d\n", results_map_fd))
+		goto cleanup;
+
+	kprobe_link = bpf_program__attach_kprobe(kprobe_prog, false,
+						 kprobe_name);
+	if (CHECK(IS_ERR(kprobe_link), "attach_kprobe",
+		  "err %ld\n", PTR_ERR(kprobe_link))) {
+		kprobe_link = NULL;
+		goto cleanup;
+	}
+
+	memset(&curr, 0, sizeof(curr));
+	in->sin_family = AF_INET;
+	in->sin_port = htons(5555);
+	in->sin_addr.s_addr = inet_addr("255.255.255.255");
+	memcpy(&orig, &curr, sizeof(curr));
+
+	sock_fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (CHECK(sock_fd < 0, "create_sock_fd", "err %d\n", sock_fd))
+		goto cleanup;
+
+	connect(sock_fd, &curr, sizeof(curr));
+	close(sock_fd);
+
+	err = bpf_map_lookup_elem(results_map_fd, &zero, &tmp);
+	if (CHECK(err, "get_kprobe_res",
+		  "failed to get kprobe res: %d\n", err))
+		goto cleanup;
+
+	in = (struct sockaddr_in *)&tmp;
+	if (CHECK(memcmp(&tmp, &orig, sizeof(orig)), "check_kprobe_res",
+		  "wrong kprobe res from probe read: %s:%u\n",
+		  inet_ntoa(in->sin_addr), ntohs(in->sin_port)))
+		goto cleanup;
+
+	memset(&tmp, 0xab, sizeof(tmp));
+
+	in = (struct sockaddr_in *)&curr;
+	if (CHECK(memcmp(&curr, &tmp, sizeof(tmp)), "check_kprobe_res",
+		  "wrong kprobe res from probe write: %s:%u\n",
+		  inet_ntoa(in->sin_addr), ntohs(in->sin_port)))
+		goto cleanup;
+cleanup:
+	bpf_link__destroy(kprobe_link);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c b/tools/testing/selftests/bpf/progs/test_probe_user.c
new file mode 100644
index 000000000000..a9b8a0bde0b9
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_probe_user.c
@@ -0,0 +1,33 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ptrace.h>
+#include <linux/bpf.h>
+
+#include <netinet/in.h>
+
+#include "bpf_helpers.h"
+#include "bpf_tracing.h"
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct sockaddr_in);
+} results_map SEC(".maps");
+
+SEC("kprobe/__sys_connect")
+int handle_sys_connect(struct pt_regs *ctx)
+{
+	void *ptr = (void *)PT_REGS_PARM2(ctx);
+	struct sockaddr_in old, new;
+	const int zero = 0;
+
+	bpf_probe_read_user(&old, sizeof(old), ptr);
+	bpf_map_update_elem(&results_map, &zero, &old, 0);
+	__builtin_memset(&new, 0xab, sizeof(new));
+	bpf_probe_write_user(ptr, &new, sizeof(new));
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";