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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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