bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Ilya Leoshkevich <iii@linux.ibm.com>
Subject: Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space
Date: Fri, 25 Oct 2019 16:35:47 -0700	[thread overview]
Message-ID: <CAEf4BzaCnUSdXzs=ey7CRF8O3rEuZfOTsXH9bMPa7eFLFW9gdw@mail.gmail.com> (raw)
In-Reply-To: <20191025223835.GF14547@pc-63.home>

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

  reply	other threads:[~2019-10-25 23:36 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-25 16:37 [PATCH bpf-next 0/5] Fix BPF probe memory helpers Daniel Borkmann
2019-10-25 16:37 ` [PATCH bpf-next 1/5] uaccess: Add non-pagefault user-space write function Daniel Borkmann
2019-10-25 21:53   ` Andrii Nakryiko
2019-10-25 22:15     ` Daniel Borkmann
2019-10-25 22:43       ` Andrii Nakryiko
2019-10-25 16:37 ` [PATCH bpf-next 2/5] bpf: Make use of probe_user_write in probe write helper Daniel Borkmann
2019-10-25 21:59   ` Andrii Nakryiko
2019-10-25 16:37 ` [PATCH bpf-next 3/5] bpf: Add probe_read_{user,kernel} and probe_read_str_{user,kernel} helpers Daniel Borkmann
2019-10-25 22:08   ` Andrii Nakryiko
2019-10-25 22:20     ` Daniel Borkmann
2019-10-25 16:37 ` [PATCH bpf-next 4/5] bpf, samples: Use bpf_probe_read_user where appropriate Daniel Borkmann
2019-10-25 22:08   ` Andrii Nakryiko
2019-10-25 16:37 ` [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space Daniel Borkmann
2019-10-25 22:14   ` Andrii Nakryiko
2019-10-25 22:38     ` Daniel Borkmann
2019-10-25 23:35       ` Andrii Nakryiko [this message]
2019-10-25 23:36   ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzaCnUSdXzs=ey7CRF8O3rEuZfOTsXH9bMPa7eFLFW9gdw@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=iii@linux.ibm.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).