* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jonathan Lemon @ 2019-06-28 20:29 UTC (permalink / raw)
To: Laatz, Kevin
Cc: Jakub Kicinski, netdev, ast, daniel, bjorn.topel, magnus.karlsson,
bpf, intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <f0ca817a-02b4-df22-d01b-7bc07171a4dc@intel.com>
On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
> On 27/06/2019 22:25, Jakub Kicinski wrote:
>> On Thu, 27 Jun 2019 12:14:50 +0100, Laatz, Kevin wrote:
>>> On the application side (xdpsock), we don't have to worry about the
>>> user
>>> defined headroom, since it is 0, so we only need to account for the
>>> XDP_PACKET_HEADROOM when computing the original address (in the
>>> default
>>> scenario).
>> That assumes specific layout for the data inside the buffer. Some
>> NICs
>> will prepend information like timestamp to the packet, meaning the
>> packet would start at offset XDP_PACKET_HEADROOM + metadata len..
>
> Yes, if NICs prepend extra data to the packet that would be a problem
> for
> using this feature in isolation. However, if we also add in support
> for in-order
> RX and TX rings, that would no longer be an issue. However, even for
> NICs
> which do prepend data, this patchset should not break anything that is
> currently
> working.
I read this as "the correct buffer address is recovered from the shadow
ring".
I'm not sure I'm comfortable with that, and I'm also not sold on
in-order completion
for the RX/TX rings.
>> I think that's very limiting. What is the challenge in providing
>> aligned addresses, exactly?
> The challenges are two-fold:
> 1) it prevents using arbitrary buffer sizes, which will be an issue
> supporting e.g. jumbo frames in future.
> 2) higher level user-space frameworks which may want to use AF_XDP,
> such as DPDK, do not currently support having buffers with 'fixed'
> alignment.
> The reason that DPDK uses arbitrary placement is that:
> - it would stop things working on certain NICs which
> need the actual writable space specified in units of 1k - therefore we
> need 2k + metadata space.
> - we place padding between buffers to avoid constantly
> hitting the same memory channels when accessing memory.
> - it allows the application to choose the actual buffer
> size it wants to use.
> We make use of the above to allow us to speed up processing
> significantly and also reduce the packet buffer memory size.
>
> Not having arbitrary buffer alignment also means an AF_XDP
> driver for DPDK cannot be a drop-in replacement for existing drivers
> in those frameworks. Even with a new capability to allow an arbitrary
> buffer alignment, existing apps will need to be modified to use that
> new capability.
Since all buffers in the umem are the same chunk size, the original
buffer
address can be recalculated with some multiply/shift math. However,
this is
more expensive than just a mask operation.
--
Jonathan
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jakub Kicinski @ 2019-06-28 20:25 UTC (permalink / raw)
To: Laatz, Kevin
Cc: Jonathan Lemon, netdev, ast, daniel, bjorn.topel, magnus.karlsson,
bpf, intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <f0ca817a-02b4-df22-d01b-7bc07171a4dc@intel.com>
On Fri, 28 Jun 2019 17:19:09 +0100, Laatz, Kevin wrote:
> On 27/06/2019 22:25, Jakub Kicinski wrote:
> > On Thu, 27 Jun 2019 12:14:50 +0100, Laatz, Kevin wrote:
> >> On the application side (xdpsock), we don't have to worry about the user
> >> defined headroom, since it is 0, so we only need to account for the
> >> XDP_PACKET_HEADROOM when computing the original address (in the default
> >> scenario).
> > That assumes specific layout for the data inside the buffer. Some NICs
> > will prepend information like timestamp to the packet, meaning the
> > packet would start at offset XDP_PACKET_HEADROOM + metadata len..
>
> Yes, if NICs prepend extra data to the packet that would be a problem for
> using this feature in isolation. However, if we also add in support for
> in-order RX and TX rings, that would no longer be an issue.
Can you shed more light on in-order rings? Do you mean that RX frames
come in order buffers were placed in the fill queue? That wouldn't
make practical sense, no? Even if the application does no
reordering there is also XDP_DROP and XDP_TX. Please explain :)
> However, even for NICs which do prepend data, this patchset should
> not break anything that is currently working.
My understanding from the beginnings of AF_XDP was that we were
searching for a format flexible enough to support most if not all NICs.
Creating an ABI which will preclude vendors from supporting DPDK via
AF_XDP would seriously undermine the neutrality aspect.
> > I think that's very limiting. What is the challenge in providing
> > aligned addresses, exactly?
> The challenges are two-fold:
> 1) it prevents using arbitrary buffer sizes, which will be an issue
> supporting e.g. jumbo frames in future.
Presumably support for jumbos would require a multi-buffer setup, and
therefore extensions to the ring format. Should we perhaps look into
implementing unaligned chunks by extending ring format as well?
> 2) higher level user-space frameworks which may want to use AF_XDP, such
> as DPDK, do not currently support having buffers with 'fixed' alignment.
> The reason that DPDK uses arbitrary placement is that:
> - it would stop things working on certain NICs which need the
> actual writable space specified in units of 1k - therefore we need 2k +
> metadata space.
> - we place padding between buffers to avoid constantly hitting
> the same memory channels when accessing memory.
> - it allows the application to choose the actual buffer size it
> wants to use.
> We make use of the above to allow us to speed up processing
> significantly and also reduce the packet buffer memory size.
>
> Not having arbitrary buffer alignment also means an AF_XDP driver
> for DPDK cannot be a drop-in replacement for existing drivers in those
> frameworks. Even with a new capability to allow an arbitrary buffer
> alignment, existing apps will need to be modified to use that new
> capability.
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix -Wstrict-aliasing in test_sockopt_sk.c
From: Song Liu @ 2019-06-28 20:25 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S . Miller, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190628011233.63680-1-sdf@google.com>
On Thu, Jun 27, 2019 at 6:14 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Let's use union with u8[4] and u32 members for sockopt buffer,
> that should fix any possible aliasing issues.
>
> test_sockopt_sk.c: In function ‘getsetsockopt’:
> test_sockopt_sk.c:115:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> if (*(__u32 *)buf != 0x55AA*2) {
> ^~
> test_sockopt_sk.c:116:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
> ^~~~~~~
>
> Fixes: 8a027dc0d8f5 ("selftests/bpf: add sockopt test that exercises sk helpers")
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/testing/selftests/bpf/test_sockopt_sk.c | 51 +++++++++----------
> 1 file changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockopt_sk.c b/tools/testing/selftests/bpf/test_sockopt_sk.c
> index 12e79ed075ce..036b652e5ca9 100644
> --- a/tools/testing/selftests/bpf/test_sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/test_sockopt_sk.c
> @@ -22,7 +22,10 @@
> static int getsetsockopt(void)
> {
> int fd, err;
> - char buf[4] = {};
> + union {
> + char u8[4];
> + __u32 u32;
> + } buf = {};
> socklen_t optlen;
>
> fd = socket(AF_INET, SOCK_STREAM, 0);
> @@ -33,31 +36,31 @@ static int getsetsockopt(void)
>
> /* IP_TOS - BPF bypass */
>
> - buf[0] = 0x08;
> - err = setsockopt(fd, SOL_IP, IP_TOS, buf, 1);
> + buf.u8[0] = 0x08;
> + err = setsockopt(fd, SOL_IP, IP_TOS, &buf, 1);
> if (err) {
> log_err("Failed to call setsockopt(IP_TOS)");
> goto err;
> }
>
> - buf[0] = 0x00;
> + buf.u8[0] = 0x00;
> optlen = 1;
> - err = getsockopt(fd, SOL_IP, IP_TOS, buf, &optlen);
> + err = getsockopt(fd, SOL_IP, IP_TOS, &buf, &optlen);
> if (err) {
> log_err("Failed to call getsockopt(IP_TOS)");
> goto err;
> }
>
> - if (buf[0] != 0x08) {
> + if (buf.u8[0] != 0x08) {
> log_err("Unexpected getsockopt(IP_TOS) buf[0] 0x%02x != 0x08",
> - buf[0]);
> + buf.u8[0]);
> goto err;
> }
>
> /* IP_TTL - EPERM */
>
> - buf[0] = 1;
> - err = setsockopt(fd, SOL_IP, IP_TTL, buf, 1);
> + buf.u8[0] = 1;
> + err = setsockopt(fd, SOL_IP, IP_TTL, &buf, 1);
> if (!err || errno != EPERM) {
> log_err("Unexpected success from setsockopt(IP_TTL)");
> goto err;
> @@ -65,16 +68,16 @@ static int getsetsockopt(void)
>
> /* SOL_CUSTOM - handled by BPF */
>
> - buf[0] = 0x01;
> - err = setsockopt(fd, SOL_CUSTOM, 0, buf, 1);
> + buf.u8[0] = 0x01;
> + err = setsockopt(fd, SOL_CUSTOM, 0, &buf, 1);
> if (err) {
> log_err("Failed to call setsockopt");
> goto err;
> }
>
> - buf[0] = 0x00;
> + buf.u32 = 0x00;
> optlen = 4;
> - err = getsockopt(fd, SOL_CUSTOM, 0, buf, &optlen);
> + err = getsockopt(fd, SOL_CUSTOM, 0, &buf, &optlen);
> if (err) {
> log_err("Failed to call getsockopt");
> goto err;
> @@ -84,37 +87,31 @@ static int getsetsockopt(void)
> log_err("Unexpected optlen %d != 1", optlen);
> goto err;
> }
> - if (buf[0] != 0x01) {
> - log_err("Unexpected buf[0] 0x%02x != 0x01", buf[0]);
> + if (buf.u8[0] != 0x01) {
> + log_err("Unexpected buf[0] 0x%02x != 0x01", buf.u8[0]);
> goto err;
> }
>
> /* SO_SNDBUF is overwritten */
>
> - buf[0] = 0x01;
> - buf[1] = 0x01;
> - buf[2] = 0x01;
> - buf[3] = 0x01;
> - err = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, buf, 4);
> + buf.u32 = 0x01010101;
> + err = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &buf, 4);
> if (err) {
> log_err("Failed to call setsockopt(SO_SNDBUF)");
> goto err;
> }
>
> - buf[0] = 0x00;
> - buf[1] = 0x00;
> - buf[2] = 0x00;
> - buf[3] = 0x00;
> + buf.u32 = 0x00;
> optlen = 4;
> - err = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, buf, &optlen);
> + err = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &buf, &optlen);
> if (err) {
> log_err("Failed to call getsockopt(SO_SNDBUF)");
> goto err;
> }
>
> - if (*(__u32 *)buf != 0x55AA*2) {
> + if (buf.u32 != 0x55AA*2) {
> log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
> - *(__u32 *)buf);
> + buf.u32);
> goto err;
> }
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH v3] bpf: fix uapi bpf_prog_info fields alignment
From: Song Liu @ 2019-06-28 20:19 UTC (permalink / raw)
To: Baruch Siach
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Networking, bpf, Dmitry V . Levin, Arnd Bergmann,
linux-arch, Jiri Olsa, Geert Uytterhoeven, Linus Torvalds
In-Reply-To: <02938ce219d535a8c7c29ce796b3d6ea59c3ed15.1561694925.git.baruch@tkos.co.il>
On Thu, Jun 27, 2019 at 9:11 PM Baruch Siach <baruch@tkos.co.il> wrote:
>
> Merge commit 1c8c5a9d38f60 ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
> fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
> applications") by taking the gpl_compatible 1-bit field definition from
> commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
> bpf_prog_info") as is. That breaks architectures with 16-bit alignment
> like m68k. Add 31-bit pad after gpl_compatible to restore alignment of
> following fields.
>
> Thanks to Dmitry V. Levin his analysis of this bug history.
>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> v3:
> Use alignment pad as Alexei Starovoitov suggested
>
> v2:
> Use anonymous union with pad to make it less likely to break again in
> the future.
> ---
> include/uapi/linux/bpf.h | 1 +
> tools/include/uapi/linux/bpf.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a8b823c30b43..29a5bc3d5c66 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3143,6 +3143,7 @@ struct bpf_prog_info {
> char name[BPF_OBJ_NAME_LEN];
> __u32 ifindex;
> __u32 gpl_compatible:1;
> + __u32 :31; /* alignment pad */
> __u64 netns_dev;
> __u64 netns_ino;
> __u32 nr_jited_ksyms;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a8b823c30b43..29a5bc3d5c66 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3143,6 +3143,7 @@ struct bpf_prog_info {
> char name[BPF_OBJ_NAME_LEN];
> __u32 ifindex;
> __u32 gpl_compatible:1;
> + __u32 :31; /* alignment pad */
> __u64 netns_dev;
> __u64 netns_ino;
> __u32 nr_jited_ksyms;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 5/9] libbpf: add tracepoint attach API
From: Song Liu @ 2019-06-28 20:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <CAPhsuW6zLEY=3tc6Ja5Cnam_RJ=eUFdfmMnN4mfjWn18O5Rbgg@mail.gmail.com>
On Fri, Jun 28, 2019 at 12:50 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Allow attaching BPF programs to kernel tracepoint BPF hooks specified by
> > category and name.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 78 ++++++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 4 +++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 83 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 65d2fef41003..76d1599a7d56 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4229,6 +4229,84 @@ struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> > return link;
> > }
> >
> > +static int determine_tracepoint_id(const char* tp_category, const char* tp_name)
> > +{
> > + char file[PATH_MAX];
> > + int ret;
> > +
> > + ret = snprintf(file, sizeof(file),
> > + "/sys/kernel/debug/tracing/events/%s/%s/id",
> > + tp_category, tp_name);
> > + if (ret < 0)
> > + return -errno;
> > + if (ret >= sizeof(file)) {
> > + pr_debug("tracepoint %s/%s path is too long\n",
> > + tp_category, tp_name);
> > + return -E2BIG;
> > + }
> > + return parse_uint_from_file(file);
> > +}
> > +
> > +static int perf_event_open_tracepoint(const char* tp_category,
> > + const char* tp_name)
> > +{
> > + struct perf_event_attr attr = {};
> > + char errmsg[STRERR_BUFSIZE];
> > + int tp_id, pfd, err;
> > +
> > + tp_id = determine_tracepoint_id(tp_category, tp_name);
> > + if (tp_id < 0){
> > + pr_warning("failed to determine tracepoint '%s/%s' perf ID: %s\n",
> > + tp_category, tp_name,
> > + libbpf_strerror_r(tp_id, errmsg, sizeof(errmsg)));
> > + return tp_id;
> > + }
> > +
> > + attr.type = PERF_TYPE_TRACEPOINT;
> > + attr.size = sizeof(attr);
> > + attr.config = tp_id;
> > +
> > + pfd = syscall( __NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + if (pfd < 0) {
> > + err = -errno;
> > + pr_warning("tracepoint '%s/%s' perf_event_open() failed: %s\n",
> > + tp_category, tp_name,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return err;
> > + }
>
> Similar to the 4/9, I guess we can remove some duplicated pr_warning().
>
> Thanks,
> Song
>
> > + return pfd;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_tracepoint(struct bpf_program *prog,
> > + const char *tp_category,
> > + const char *tp_name)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link *link;
> > + int pfd, err;
> > +
> > + pfd = perf_event_open_tracepoint(tp_category, tp_name);
> > + if (pfd < 0) {
> > + pr_warning("program '%s': failed to create tracepoint '%s/%s' perf event: %s\n",
> > + bpf_program__title(prog, false),
> > + tp_category, tp_name,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link = bpf_program__attach_perf_event(prog, pfd);
> > + if (IS_ERR(link)) {
> > + close(pfd);
> > + err = PTR_ERR(link);
> > + pr_warning("program '%s': failed to attach to tracepoint '%s/%s': %s\n",
> > + bpf_program__title(prog, false),
> > + tp_category, tp_name,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return link;
> > + }
> > + return link;
> > +}
> > +
> > enum bpf_perf_event_ret
> > bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> > void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index bd767cc11967..60611f4b4e1d 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -178,6 +178,10 @@ LIBBPF_API struct bpf_link *
> > bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> > pid_t pid, const char *binary_path,
> > size_t func_offset);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_tracepoint(struct bpf_program *prog,
> > + const char *tp_category,
> > + const char *tp_name);
> >
> > struct bpf_insn;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 57a40fb60718..3c618b75ef65 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -171,6 +171,7 @@ LIBBPF_0.0.4 {
> > bpf_object__load_xattr;
> > bpf_program__attach_kprobe;
> > bpf_program__attach_perf_event;
> > + bpf_program__attach_tracepoint;
> > bpf_program__attach_uprobe;
> > btf_dump__dump_type;
> > btf_dump__free;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH v3 bpf-next 9/9] selftests/bpf: convert existing tracepoint tests to new APIs
From: Song Liu @ 2019-06-28 20:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-10-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Convert some existing tests that attach to tracepoints to use
> bpf_program__attach_tracepoint API instead.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> .../bpf/prog_tests/stacktrace_build_id.c | 50 ++++---------------
> .../selftests/bpf/prog_tests/stacktrace_map.c | 43 ++++------------
> .../bpf/prog_tests/stacktrace_map_raw_tp.c | 15 ++++--
> 3 files changed, 31 insertions(+), 77 deletions(-)
More deletions. Nice!
^ permalink raw reply
* Re: [PATCH v3 bpf-next 8/9] selftests/bpf: add kprobe/uprobe selftests
From: Song Liu @ 2019-06-28 20:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-9-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:54 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Add tests verifying kprobe/kretprobe/uprobe/uretprobe APIs work as
> expected.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> .../selftests/bpf/prog_tests/attach_probe.c | 155 ++++++++++++++++++
> .../selftests/bpf/progs/test_attach_probe.c | 55 +++++++
> 2 files changed, 210 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_probe.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_attach_probe.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> new file mode 100644
> index 000000000000..f22929063c58
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +
> +ssize_t get_base_addr() {
> + size_t start;
> + char buf[256];
> + FILE *f;
> +
> + f = fopen("/proc/self/maps", "r");
> + if (!f)
> + return -errno;
> +
> + while (fscanf(f, "%zx-%*x %s %*s\n", &start, buf) == 2) {
> + if (strcmp(buf, "r-xp") == 0) {
> + fclose(f);
> + return start;
> + }
> + }
> +
> + fclose(f);
> + return -EINVAL;
> +}
> +
> +void test_attach_probe(void)
> +{
> + const char *kprobe_name = "kprobe/sys_nanosleep";
> + const char *kretprobe_name = "kretprobe/sys_nanosleep";
> + const char *uprobe_name = "uprobe/trigger_func";
> + const char *uretprobe_name = "uretprobe/trigger_func";
> + const int kprobe_idx = 0, kretprobe_idx = 1;
> + const int uprobe_idx = 2, uretprobe_idx = 3;
> + const char *file = "./test_attach_probe.o";
> + struct bpf_program *kprobe_prog, *kretprobe_prog;
> + struct bpf_program *uprobe_prog, *uretprobe_prog;
> + struct bpf_object *obj;
> + int err, prog_fd, duration = 0, res;
> + struct bpf_link *kprobe_link = NULL;
> + struct bpf_link *kretprobe_link = NULL;
> + struct bpf_link *uprobe_link = NULL;
> + struct bpf_link *uretprobe_link = NULL;
> + int results_map_fd;
> + size_t uprobe_offset;
> + ssize_t base_addr;
> +
> + base_addr = get_base_addr();
> + if (CHECK(base_addr < 0, "get_base_addr",
> + "failed to find base addr: %zd", base_addr))
> + return;
> + uprobe_offset = (size_t)&get_base_addr - base_addr;
> +
> + /* load programs */
> + err = bpf_prog_load(file, BPF_PROG_TYPE_KPROBE, &obj, &prog_fd);
> + if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
> + return;
> +
> + kprobe_prog = bpf_object__find_program_by_title(obj, kprobe_name);
> + if (CHECK(!kprobe_prog, "find_probe",
> + "prog '%s' not found\n", kprobe_name))
> + goto cleanup;
> + kretprobe_prog = bpf_object__find_program_by_title(obj, kretprobe_name);
> + if (CHECK(!kretprobe_prog, "find_probe",
> + "prog '%s' not found\n", kretprobe_name))
> + goto cleanup;
> + uprobe_prog = bpf_object__find_program_by_title(obj, uprobe_name);
> + if (CHECK(!uprobe_prog, "find_probe",
> + "prog '%s' not found\n", uprobe_name))
> + goto cleanup;
> + uretprobe_prog = bpf_object__find_program_by_title(obj, uretprobe_name);
> + if (CHECK(!uretprobe_prog, "find_probe",
> + "prog '%s' not found\n", uretprobe_name))
> + goto cleanup;
> +
> + /* load maps */
> + 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 /* retprobe */,
> + "sys_nanosleep");
> + if (CHECK(IS_ERR(kprobe_link), "attach_kprobe",
> + "err %ld\n", PTR_ERR(kprobe_link)))
> + goto cleanup;
> +
> + kretprobe_link = bpf_program__attach_kprobe(kretprobe_prog,
> + true /* retprobe */,
> + "sys_nanosleep");
> + if (CHECK(IS_ERR(kretprobe_link), "attach_kretprobe",
> + "err %ld\n", PTR_ERR(kretprobe_link)))
> + goto cleanup;
> +
> + uprobe_link = bpf_program__attach_uprobe(uprobe_prog,
> + false /* retprobe */,
> + 0 /* self pid */,
> + "/proc/self/exe",
> + uprobe_offset);
> + if (CHECK(IS_ERR(uprobe_link), "attach_uprobe",
> + "err %ld\n", PTR_ERR(uprobe_link)))
> + goto cleanup;
> +
> + uretprobe_link = bpf_program__attach_uprobe(uretprobe_prog,
> + true /* retprobe */,
> + -1 /* any pid */,
> + "/proc/self/exe",
> + uprobe_offset);
> + if (CHECK(IS_ERR(uretprobe_link), "attach_uretprobe",
> + "err %ld\n", PTR_ERR(uretprobe_link)))
> + goto cleanup;
> +
> + /* trigger & validate kprobe && kretprobe */
> + usleep(1);
> +
> + err = bpf_map_lookup_elem(results_map_fd, &kprobe_idx, &res);
> + if (CHECK(err, "get_kprobe_res",
> + "failed to get kprobe res: %d\n", err))
> + goto cleanup;
> + if (CHECK(res != kprobe_idx + 1, "check_kprobe_res",
> + "wrong kprobe res: %d\n", res))
> + goto cleanup;
> +
> + err = bpf_map_lookup_elem(results_map_fd, &kretprobe_idx, &res);
> + if (CHECK(err, "get_kretprobe_res",
> + "failed to get kretprobe res: %d\n", err))
> + goto cleanup;
> + if (CHECK(res != kretprobe_idx + 1, "check_kretprobe_res",
> + "wrong kretprobe res: %d\n", res))
> + goto cleanup;
> +
> + /* trigger & validate uprobe & uretprobe */
> + get_base_addr();
> +
> + err = bpf_map_lookup_elem(results_map_fd, &uprobe_idx, &res);
> + if (CHECK(err, "get_uprobe_res",
> + "failed to get uprobe res: %d\n", err))
> + goto cleanup;
> + if (CHECK(res != uprobe_idx + 1, "check_uprobe_res",
> + "wrong uprobe res: %d\n", res))
> + goto cleanup;
> +
> + err = bpf_map_lookup_elem(results_map_fd, &uretprobe_idx, &res);
> + if (CHECK(err, "get_uretprobe_res",
> + "failed to get uretprobe res: %d\n", err))
> + goto cleanup;
> + if (CHECK(res != uretprobe_idx + 1, "check_uretprobe_res",
> + "wrong uretprobe res: %d\n", res))
> + goto cleanup;
> +
> +cleanup:
> + bpf_link__destroy(kprobe_link);
> + bpf_link__destroy(kretprobe_link);
> + bpf_link__destroy(uprobe_link);
> + bpf_link__destroy(uretprobe_link);
> + bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> new file mode 100644
> index 000000000000..7a7c5cd728c8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Facebook
> +
> +#include <linux/ptrace.h>
> +#include <linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +struct {
> + int type;
> + int max_entries;
> + int *key;
> + int *value;
> +} results_map SEC(".maps") = {
> + .type = BPF_MAP_TYPE_ARRAY,
> + .max_entries = 4,
> +};
> +
> +SEC("kprobe/sys_nanosleep")
> +int handle_sys_nanosleep_entry(struct pt_regs *ctx)
> +{
> + const int key = 0, value = 1;
> +
> + bpf_map_update_elem(&results_map, &key, &value, 0);
> + return 0;
> +}
> +
> +SEC("kretprobe/sys_nanosleep")
> +int handle_sys_getpid_return(struct pt_regs *ctx)
> +{
> + const int key = 1, value = 2;
> +
> + bpf_map_update_elem(&results_map, &key, &value, 0);
> + return 0;
> +}
> +
> +SEC("uprobe/trigger_func")
> +int handle_uprobe_entry(struct pt_regs *ctx)
> +{
> + const int key = 2, value = 3;
> +
> + bpf_map_update_elem(&results_map, &key, &value, 0);
> + return 0;
> +}
> +
> +SEC("uretprobe/trigger_func")
> +int handle_uprobe_return(struct pt_regs *ctx)
> +{
> + const int key = 3, value = 4;
> +
> + bpf_map_update_elem(&results_map, &key, &value, 0);
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> +__u32 _version SEC("version") = 1;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 7/9] selftests/bpf: switch test to new attach_perf_event API
From: Song Liu @ 2019-06-28 20:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-8-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Use new bpf_program__attach_perf_event() in test previously relying on
> direct ioctl manipulations.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> .../bpf/prog_tests/stacktrace_build_id_nmi.c | 31 +++++++++----------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> index 1c1a2f75f3d8..9557b7dfb782 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> @@ -17,6 +17,7 @@ static __u64 read_perf_max_sample_freq(void)
> void test_stacktrace_build_id_nmi(void)
> {
> int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
> + const char *prog_name = "tracepoint/random/urandom_read";
> const char *file = "./test_stacktrace_build_id.o";
> int err, pmu_fd, prog_fd;
> struct perf_event_attr attr = {
> @@ -25,7 +26,9 @@ void test_stacktrace_build_id_nmi(void)
> .config = PERF_COUNT_HW_CPU_CYCLES,
> };
> __u32 key, previous_key, val, duration = 0;
> + struct bpf_program *prog;
> struct bpf_object *obj;
> + struct bpf_link *link;
> char buf[256];
> int i, j;
> struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> @@ -39,6 +42,10 @@ void test_stacktrace_build_id_nmi(void)
> if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
> return;
>
> + prog = bpf_object__find_program_by_title(obj, prog_name);
> + if (CHECK(!prog, "find_prog", "prog '%s' not found\n", prog_name))
> + goto close_prog;
> +
> pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
> 0 /* cpu 0 */, -1 /* group id */,
> 0 /* flags */);
> @@ -47,15 +54,12 @@ void test_stacktrace_build_id_nmi(void)
> pmu_fd, errno))
> goto close_prog;
>
> - err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> - if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n",
> - err, errno))
> - goto close_pmu;
> -
> - err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> - if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n",
> - err, errno))
> - goto disable_pmu;
> + link = bpf_program__attach_perf_event(prog, pmu_fd);
> + if (CHECK(IS_ERR(link), "attach_perf_event",
> + "err %ld\n", PTR_ERR(link))) {
> + close(pmu_fd);
> + goto close_prog;
> + }
>
> /* find map fds */
> control_map_fd = bpf_find_map(__func__, obj, "control_map");
> @@ -134,8 +138,7 @@ void test_stacktrace_build_id_nmi(void)
> * try it one more time.
> */
> if (build_id_matches < 1 && retry--) {
> - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> - close(pmu_fd);
> + bpf_link__destroy(link);
> bpf_object__close(obj);
> printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> __func__);
> @@ -154,11 +157,7 @@ void test_stacktrace_build_id_nmi(void)
> */
>
> disable_pmu:
> - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> -
> -close_pmu:
> - close(pmu_fd);
> -
> + bpf_link__destroy(link);
> close_prog:
> bpf_object__close(obj);
> }
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Song Liu @ 2019-06-28 20:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <CAEf4Bzbo4r9=VZ2kYaOsZa7HHvjXeEw4uWXhpjcUDvazOcKrzw@mail.gmail.com>
On Fri, Jun 28, 2019 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >
> > On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
> > >
> > > Add ability to attach to kernel and user probes and retprobes.
> > > Implementation depends on perf event support for kprobes/uprobes.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > tools/lib/bpf/libbpf.c | 213 +++++++++++++++++++++++++++++++++++++++
> > > tools/lib/bpf/libbpf.h | 7 ++
> > > tools/lib/bpf/libbpf.map | 2 +
> > > 3 files changed, 222 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 606705f878ba..65d2fef41003 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -4016,6 +4016,219 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> > > return (struct bpf_link *)link;
> > > }
> > >
> > > +static int parse_uint(const char *buf)
> > > +{
> > > + int ret;
> > > +
> > > + errno = 0;
> > > + ret = (int)strtol(buf, NULL, 10);
> > > + if (errno) {
> > > + ret = -errno;
> > > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > > + return ret;
> > > + }
> > > + if (ret < 0) {
> > > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > > + return -EINVAL;
> > > + }
> > > + return ret;
> > > +}
> > > +
> > > +static int parse_uint_from_file(const char* file)
> > > +{
> > > + char buf[STRERR_BUFSIZE];
> > > + int fd, ret;
> > > +
> > > + fd = open(file, O_RDONLY);
> > > + if (fd < 0) {
> > > + ret = -errno;
> > > + pr_debug("failed to open '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + ret = read(fd, buf, sizeof(buf));
> > > + ret = ret < 0 ? -errno : ret;
> > > + close(fd);
> > > + if (ret < 0) {
> > > + pr_debug("failed to read '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + if (ret == 0 || ret >= sizeof(buf)) {
> > > + buf[sizeof(buf) - 1] = 0;
> > > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > > + return -EINVAL;
> > > + }
> > > + return parse_uint(buf);
> > > +}
> > > +
> > > +static int determine_kprobe_perf_type(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> > > + return parse_uint_from_file(file);
> > > +}
> > > +
> > > +static int determine_uprobe_perf_type(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > > + return parse_uint_from_file(file);
> > > +}
> > > +
> > > +static int parse_config_from_file(const char *file)
> > > +{
> > > + char buf[STRERR_BUFSIZE];
> > > + int fd, ret;
> > > +
> > > + fd = open(file, O_RDONLY);
> > > + if (fd < 0) {
> > > + ret = -errno;
> > > + pr_debug("failed to open '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + ret = read(fd, buf, sizeof(buf));
> > > + ret = ret < 0 ? -errno : ret;
> > > + close(fd);
> > > + if (ret < 0) {
> > > + pr_debug("failed to read '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + if (ret == 0 || ret >= sizeof(buf)) {
> > > + buf[sizeof(buf) - 1] = 0;
> > > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > > + return -EINVAL;
> > > + }
> > > + if (strncmp(buf, "config:", 7)) {
> > > + pr_debug("expected 'config:' prefix, found '%s'\n", buf);
> > > + return -EINVAL;
> > > + }
> > > + return parse_uint(buf + 7);
> > > +}
> > > +
> > > +static int determine_kprobe_retprobe_bit(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> > > + return parse_config_from_file(file);
> > > +}
> > > +
> > > +static int determine_uprobe_retprobe_bit(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > > + return parse_config_from_file(file);
> > > +}
> >
> > Can we do the above with fscanf? Would that be easier?
>
> It would be less code, but also less strict semantics. E.g., fscanf
> would happily leave out any garbage after number (e.g., 123blablabla,
> would still parse). Also, from brief googling, fscanf doesn't handle
> overflows well.
>
> So I guess I'd vote for this more verbose, but also more strict
> checking, unless you insist on fscanf.
I don't think we need to worry about kernel giving garbage in sysfs.
Most common error gonna be the file doesn't exist. Error messages
like "Failed to parse <filename>" would be sufficient.
Let's keep it simpler.
>
> >
> > > +
> > > +static int perf_event_open_probe(bool uprobe, bool retprobe, const char* name,
> > > + uint64_t offset, int pid)
> > > +{
> > > + struct perf_event_attr attr = {};
> > > + char errmsg[STRERR_BUFSIZE];
> > > + int type, pfd, err;
> > > +
> > > + type = uprobe ? determine_uprobe_perf_type()
> > > + : determine_kprobe_perf_type();
> > > + if (type < 0) {
> > > + pr_warning("failed to determine %s perf type: %s\n",
> > > + uprobe ? "uprobe" : "kprobe",
> > > + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > > + return type;
> > > + }
> > > + if (retprobe) {
> > > + int bit = uprobe ? determine_uprobe_retprobe_bit()
> > > + : determine_kprobe_retprobe_bit();
> > > +
> > > + if (bit < 0) {
> > > + pr_warning("failed to determine %s retprobe bit: %s\n",
> > > + uprobe ? "uprobe" : "kprobe",
> > > + libbpf_strerror_r(bit, errmsg,
> > > + sizeof(errmsg)));
> > > + return bit;
> > > + }
> > > + attr.config |= 1 << bit;
> > > + }
> > > + attr.size = sizeof(attr);
> > > + attr.type = type;
> > > + attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> > > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > > +
> > > + /* pid filter is meaningful only for uprobes */
> > > + pfd = syscall(__NR_perf_event_open, &attr,
> > > + pid < 0 ? -1 : pid /* pid */,
> > > + pid == -1 ? 0 : -1 /* cpu */,
> > > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > > + if (pfd < 0) {
> > > + err = -errno;
> > > + pr_warning("%s perf_event_open() failed: %s\n",
> > > + uprobe ? "uprobe" : "kprobe",
> > > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> >
> > We have another warning in bpf_program__attach_[k|u]probe(). I guess
> > we can remove this one here.
>
> This points specifically to perf_event_open() failing versus other
> possible failures. Messages in attach_{k,u}probe won't have that, they
> will repeat more generic "failed to attach" message. Believe me, if
> something goes wrong in libbpf, I'd rather have too much logging than
> too little :)
>
Fair enough. Let's be verbose here. :)
Song
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jakub Kicinski @ 2019-06-28 20:08 UTC (permalink / raw)
To: Björn Töpel
Cc: Laatz, Kevin, Jonathan Lemon, netdev, ast, daniel,
magnus.karlsson, bpf, intel-wired-lan, bruce.richardson,
ciara.loftus
In-Reply-To: <f6fb0870-b5b4-9aba-bfb5-b4248a95da79@intel.com>
On Fri, 28 Jun 2019 18:51:37 +0200, Björn Töpel wrote:
> In your example Jakub, how would this look in XDP? Wouldn't the
> timestamp be part of the metadata (xdp_md.data_meta)? Isn't
> data-data_meta (if valid) <= XDP_PACKET_HEADROOM? That was my assumption.
The driver parses the metadata and copies it outside of the prepend
before XDP runs. Then XDP runs unaware of the prepend contents.
That's the current situation.
XDP_PACKET_HEADROOM is before the entire frame. Like this:
buffer start
/ DMA addr given to the device
/ /
v v
| XDP_HEADROOM | meta data | packet data |
Length of meta data comes in the standard fixed size descriptor.
The metadata prepend is in TV form ("TLV with no length field", length's
implied by type).
> There were some discussion on having meta data length in the struct
> xdp_desc, before AF_XDP was merged, but the conclusion was that this was
> *not* needed, because AF_XDP and the XDP program had an implicit
> contract. If you're running AF_XDP, you also have an XDP program running
> and you can determine the meta data length (and also getting back the
> original buffer).
>
> So, today in AF_XDP if XDP metadata is added, the userland application
> can look it up before the xdp_desc.addr (just like regular XDP), and how
> the XDP/AF_XDP application determines length/layout of the metadata i
> out-of-band/not specified.
>
> This is a bit messy/handwavy TBH, so maybe adding the length to the
> descriptor *is* a good idea (extending the options part of the
> xdp_desc)? Less clean though. OTOH the layout of the meta data still
> need to be determined.
Right, the device prepend is not exposed as metadata to XDP.
^ permalink raw reply
* Re: [net-next 1/4] gve: Add basic driver framework for Compute Engine Virtual NIC
From: Andrew Lunn @ 2019-06-28 20:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Catherine Sullivan, netdev, Sagi Shahar, Jon Olson,
Willem de Bruijn, Luigi Rizzo
In-Reply-To: <20190628114615.4fc81791@cakuba.netronome.com>
On Fri, Jun 28, 2019 at 11:46:15AM -0700, Jakub Kicinski wrote:
> On Fri, 28 Jun 2019 10:52:27 -0700, Catherine Sullivan wrote:
> > > > +if NET_VENDOR_GOOGLE
> > > > +
> > > > +config GVE
> > > > + tristate "Google Virtual NIC (gVNIC) support"
> > > > + depends on (PCI_MSI && X86)
> > >
> > > We usually prefer for drivers not to depend on the platform unless
> > > really necessary, but IDK how that applies to the curious new world
> > > of NICs nobody can buy :)
> >
> > This is the only platform it will ever need to run on so we would really
> > prefer to not have to support others :)
>
> I think the motivation is partially to force the uniform use of generic
> APIs across the drivers, so that re-factoring of core code is easier.
> Do you have any specific pain-points in mind where x86 dependency
> simplifies things? If not I think it's a better default to not have it.
> Not a big deal, though.
And maybe sometime in the future you might want to put this interface
in an ARM64 server?
One 'pain-paint' is that the driver might assume cache-coherency,
which is an x86 thing. If the generic APIs have been used, it should
not be an issue, but i've not spent the time to see if the DMA API has
been used correctly.
Andrew
^ permalink raw reply
* Re: [PATCH 4/4] ipvs: reduce kernel stack usage
From: Willem de Bruijn @ 2019-06-28 19:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, Wensong Zhang, Simon Horman, Julian Anastasov,
David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
James Smart, Dick Kennedy, James E . J . Bottomley,
Martin K . Petersen, Larry Finger, Florian Schilhabel,
Greg Kroah-Hartman, James Morris, linux-scsi, linux-kernel, devel,
Network Development, lvs-devel, netfilter-devel, coreteam,
Ard Biesheuvel
In-Reply-To: <20190628123819.2785504-4-arnd@arndb.de>
On Fri, Jun 28, 2019 at 8:40 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> With the new CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL option, the stack
> usage in the ipvs debug output grows because each instance of
> IP_VS_DBG_BUF() now has its own buffer of 160 bytes that add up
> rather than reusing the stack slots:
>
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_sched_persist':
> net/netfilter/ipvs/ip_vs_core.c:427:1: error: the frame size of 1052 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_new_conn_out':
> net/netfilter/ipvs/ip_vs_core.c:1231:1: error: the frame size of 1048 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_out':
> net/netfilter/ipvs/ip_vs_ftp.c:397:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_in':
> net/netfilter/ipvs/ip_vs_ftp.c:555:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Since printk() already has a way to print IPv4/IPv6 addresses using
> the %pIS format string, use that instead,
since these are sockaddr_in and sockaddr_in6, should that have the 'n'
specifier to denote network byteorder?
> combined with a macro that
> creates a local sockaddr structure on the stack. These will still
> add up, but the stack frames are now under 200 bytes.
would it make sense to just define a helper function that takes const
char * level and msg strings and up to three struct nf_inet_addr* and
do the conversion in there? No need for macros and no state on the
stack outside error paths at all.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I'm not sure this actually does what I think it does. Someone
> needs to verify that we correctly print the addresses here.
> I've also only added three files that caused the warning messages
> to be reported. There are still a lot of other instances of
> IP_VS_DBG_BUF() that could be converted the same way after the
> basic idea is confirmed.
> ---
> include/net/ip_vs.h | 71 +++++++++++++++++++--------------
> net/netfilter/ipvs/ip_vs_core.c | 44 ++++++++++----------
> net/netfilter/ipvs/ip_vs_ftp.c | 20 +++++-----
> 3 files changed, 72 insertions(+), 63 deletions(-)
>
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 3759167f91f5..3dfbeef67be6 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -227,6 +227,16 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> sizeof(ip_vs_dbg_buf), addr, \
> &ip_vs_dbg_idx)
>
> +#define IP_VS_DBG_SOCKADDR4(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in) \
> + { .sin_family = (fam), .sin_addr = (addr)->in, .sin_port = (port) }
might as well set .sin_family = AF_INET here and AF_INET6 below?
> +#define IP_VS_DBG_SOCKADDR6(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in6) \
> + { .sin6_family = (fam), .sin6_addr = (addr)->in6, .sin6_port = (port) }
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Andrii Nakryiko @ 2019-06-28 19:59 UTC (permalink / raw)
To: Song Liu
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <CAPhsuW6UMdHidpmgRzM0sZaGc5gZAnT1B7vCJVt-MrLCMjOdig@mail.gmail.com>
On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Add ability to attach to kernel and user probes and retprobes.
> > Implementation depends on perf event support for kprobes/uprobes.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 213 +++++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 7 ++
> > tools/lib/bpf/libbpf.map | 2 +
> > 3 files changed, 222 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 606705f878ba..65d2fef41003 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4016,6 +4016,219 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> > return (struct bpf_link *)link;
> > }
> >
> > +static int parse_uint(const char *buf)
> > +{
> > + int ret;
> > +
> > + errno = 0;
> > + ret = (int)strtol(buf, NULL, 10);
> > + if (errno) {
> > + ret = -errno;
> > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > + return ret;
> > + }
> > + if (ret < 0) {
> > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > + return -EINVAL;
> > + }
> > + return ret;
> > +}
> > +
> > +static int parse_uint_from_file(const char* file)
> > +{
> > + char buf[STRERR_BUFSIZE];
> > + int fd, ret;
> > +
> > + fd = open(file, O_RDONLY);
> > + if (fd < 0) {
> > + ret = -errno;
> > + pr_debug("failed to open '%s': %s\n", file,
> > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > + return ret;
> > + }
> > + ret = read(fd, buf, sizeof(buf));
> > + ret = ret < 0 ? -errno : ret;
> > + close(fd);
> > + if (ret < 0) {
> > + pr_debug("failed to read '%s': %s\n", file,
> > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > + return ret;
> > + }
> > + if (ret == 0 || ret >= sizeof(buf)) {
> > + buf[sizeof(buf) - 1] = 0;
> > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > + return -EINVAL;
> > + }
> > + return parse_uint(buf);
> > +}
> > +
> > +static int determine_kprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> > + return parse_uint_from_file(file);
> > +}
> > +
> > +static int determine_uprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > + return parse_uint_from_file(file);
> > +}
> > +
> > +static int parse_config_from_file(const char *file)
> > +{
> > + char buf[STRERR_BUFSIZE];
> > + int fd, ret;
> > +
> > + fd = open(file, O_RDONLY);
> > + if (fd < 0) {
> > + ret = -errno;
> > + pr_debug("failed to open '%s': %s\n", file,
> > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > + return ret;
> > + }
> > + ret = read(fd, buf, sizeof(buf));
> > + ret = ret < 0 ? -errno : ret;
> > + close(fd);
> > + if (ret < 0) {
> > + pr_debug("failed to read '%s': %s\n", file,
> > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > + return ret;
> > + }
> > + if (ret == 0 || ret >= sizeof(buf)) {
> > + buf[sizeof(buf) - 1] = 0;
> > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > + return -EINVAL;
> > + }
> > + if (strncmp(buf, "config:", 7)) {
> > + pr_debug("expected 'config:' prefix, found '%s'\n", buf);
> > + return -EINVAL;
> > + }
> > + return parse_uint(buf + 7);
> > +}
> > +
> > +static int determine_kprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> > + return parse_config_from_file(file);
> > +}
> > +
> > +static int determine_uprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > + return parse_config_from_file(file);
> > +}
>
> Can we do the above with fscanf? Would that be easier?
It would be less code, but also less strict semantics. E.g., fscanf
would happily leave out any garbage after number (e.g., 123blablabla,
would still parse). Also, from brief googling, fscanf doesn't handle
overflows well.
So I guess I'd vote for this more verbose, but also more strict
checking, unless you insist on fscanf.
>
> > +
> > +static int perf_event_open_probe(bool uprobe, bool retprobe, const char* name,
> > + uint64_t offset, int pid)
> > +{
> > + struct perf_event_attr attr = {};
> > + char errmsg[STRERR_BUFSIZE];
> > + int type, pfd, err;
> > +
> > + type = uprobe ? determine_uprobe_perf_type()
> > + : determine_kprobe_perf_type();
> > + if (type < 0) {
> > + pr_warning("failed to determine %s perf type: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > + return type;
> > + }
> > + if (retprobe) {
> > + int bit = uprobe ? determine_uprobe_retprobe_bit()
> > + : determine_kprobe_retprobe_bit();
> > +
> > + if (bit < 0) {
> > + pr_warning("failed to determine %s retprobe bit: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(bit, errmsg,
> > + sizeof(errmsg)));
> > + return bit;
> > + }
> > + attr.config |= 1 << bit;
> > + }
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > +
> > + /* pid filter is meaningful only for uprobes */
> > + pfd = syscall(__NR_perf_event_open, &attr,
> > + pid < 0 ? -1 : pid /* pid */,
> > + pid == -1 ? 0 : -1 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + if (pfd < 0) {
> > + err = -errno;
> > + pr_warning("%s perf_event_open() failed: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
>
> We have another warning in bpf_program__attach_[k|u]probe(). I guess
> we can remove this one here.
This points specifically to perf_event_open() failing versus other
possible failures. Messages in attach_{k,u}probe won't have that, they
will repeat more generic "failed to attach" message. Believe me, if
something goes wrong in libbpf, I'd rather have too much logging than
too little :)
>
> > + return err;
> > + }
> > + return pfd;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> > + bool retprobe,
> > + const char *func_name)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link *link;
> > + int pfd, err;
> > +
> > + pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> > + 0 /* offset */, -1 /* pid */);
> > + if (pfd < 0) {
> > + pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "kretprobe" : "kprobe", func_name,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link = bpf_program__attach_perf_event(prog, pfd);
> > + if (IS_ERR(link)) {
> > + close(pfd);
> > + err = PTR_ERR(link);
> > + pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "kretprobe" : "kprobe", func_name,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return link;
> > + }
> > + return link;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> > + bool retprobe, pid_t pid,
> > + const char *binary_path,
> > + size_t func_offset)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link *link;
> > + int pfd, err;
> > +
> > + pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> > + binary_path, func_offset, pid);
> > + if (pfd < 0) {
> > + pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "uretprobe" : "uprobe",
> > + binary_path, func_offset,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link = bpf_program__attach_perf_event(prog, pfd);
> > + if (IS_ERR(link)) {
> > + close(pfd);
> > + err = PTR_ERR(link);
> > + pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "uretprobe" : "uprobe",
> > + binary_path, func_offset,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return link;
> > + }
> > + return link;
> > +}
> > +
> > enum bpf_perf_event_ret
> > bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> > void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 1bf66c4a9330..bd767cc11967 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
> >
> > LIBBPF_API struct bpf_link *
> > bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> > + const char *func_name);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> > + pid_t pid, const char *binary_path,
> > + size_t func_offset);
> >
> > struct bpf_insn;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 756f5aa802e9..57a40fb60718 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
> > global:
> > bpf_link__destroy;
> > bpf_object__load_xattr;
> > + bpf_program__attach_kprobe;
> > bpf_program__attach_perf_event;
> > + bpf_program__attach_uprobe;
> > btf_dump__dump_type;
> > btf_dump__free;
> > btf_dump__new;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH net-next] af_packet: convert pending frame counter to atomic_t
From: Neil Horman @ 2019-06-28 19:54 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, David S. Miller, Willem de Bruijn,
Daniel Borkmann
In-Reply-To: <CAF=yD-Joh1ne4Y_pwDv8VOcWnKP-2veeXWw=eUBoZKr5___3TA@mail.gmail.com>
On Fri, Jun 28, 2019 at 11:15:09AM -0400, Willem de Bruijn wrote:
> On Fri, Jun 28, 2019 at 10:53 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > The AF_PACKET protocol, when running as a memory mapped socket uses a
> > pending frame counter to track the number of skbs in flight during
> > transmission. It is incremented during the sendmsg call (via
> > tpacket_snd), and decremented (possibly asynchronously) in the skb
> > desructor during skb_free.
> >
> > The counter is currently implemented as a percpu variable for each open
> > socket, but for reads (via packet_read_pending), we iterate over every
> > cpu variable, accumulating the total pending count.
> >
> > Given that the socket transmit path is an exclusive path (locked via the
> > pg_vec_lock mutex), we do not have the ability to increment this counter
> > on multiple cpus in parallel. This implementation also seems to have
> > the potential to be broken, in that, should an skb be freed on a cpu
> > other than the one that it was initially transmitted on, we may
> > decrement a counter that was not initially incremented, leading to
> > underflow.
> >
> > As such, adjust the packet socket struct to convert the per-cpu counter
> > to an atomic_t variable (to enforce consistency between the send path
> > and the skb free path). This saves us some space in the packet_sock
> > structure, prevents the possibility of underflow, and should reduce the
> > run time of packet_read_pending, as we only need to read a single
> > variable, instead of having to loop over every available cpu variable
> > instance.
> >
> > Tested by myself by running a small program which sends frames via
> > AF_PACKET on multiple cpus in parallel, with good results.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Willem de Bruijn <willemb@google.com>
> > ---
>
> This essentially is a revert of commit b013840810c2 ("packet: use
> percpu mmap tx frame pending refcount"). That has some benchmark
> numbers and also discusses the overflow issue.
>
Yes it is. I was looking at it thinking that the accumulation issue was more
serious now that we actually sleep until we get a completion, rather than just
schedule. I.e if we're freeing frames in parallel, I was thinking it was
possible for the ordering to result in neither instance getting a return value
from packet_read_pending of 0, which would lead to a hang/maximal timeout, but
as I look back, I think I was wrong about that.
As for the performance, you're right. They're almost the same, but I did some
perf runs, and for 10000 iterations this patch is about 0.1% slower (measuring
system time). I kind of wonder if it isn't worth the code and data savings, but
faster is faster. Though thats on a 6 cpu system. I suppose some more testing
might be warranted on high cpu count systems (i.e. is there a point at which the
looping over the cpu_possible_mask becomes more expensive), though perhaps that
just so small it doesn't matter.
> I think more interesting would be to eschew the counter when
> MSG_DONTWAIT, as it is only used to know when to exit the loop if
> need_wait.
>
Yeah, that might be interesting. Though that would mean needing to have
tpacket_destruct_skb be aware of wether or not the frame being free was sent as
part of a WAIT/DONTWAIT flagged call.
> But, IMHO packet sockets are deprecated in favor of AF_XDP and
> should be limited to bug fixes at this point.
>
I don't see any documentation listing AF_PACKET as deprecated.
People can use AF_XDP to do raw frame sends if they like,
but AF_PACKET will still be around (ostensibly in perpetuity), to support
existing applications. I see no need to avoid improving it when we can.
Neil
> > net/packet/af_packet.c | 40 +++++-----------------------------------
> > net/packet/internal.h | 2 +-
> > 2 files changed, 6 insertions(+), 36 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 8d54f3047768..25ffb486fac9 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -1154,43 +1154,17 @@ static void packet_increment_head(struct packet_ring_buffer *buff)
> >
> > static void packet_inc_pending(struct packet_ring_buffer *rb)
> > {
> > - this_cpu_inc(*rb->pending_refcnt);
> > + atomic_inc(&rb->pending_refcnt);
> > }
>
> If making this change, can also remove these helper functions. The
> layer of indirection just hinders readability.
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 6/9] libbpf: add raw tracepoint attach API
From: Song Liu @ 2019-06-28 19:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-7-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Add a wrapper utilizing bpf_link "infrastructure" to allow attaching BPF
> programs to raw tracepoints.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: [PATCH 3/4] staging: rtl8712: reduce stack usage, again
From: Willem de Bruijn @ 2019-06-28 19:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, Larry Finger, Florian Schilhabel, Greg Kroah-Hartman,
James Smart, Dick Kennedy, James E . J . Bottomley,
Martin K . Petersen, David S . Miller, Wensong Zhang,
Simon Horman, Julian Anastasov, Pablo Neira Ayuso, James Morris,
linux-scsi, linux-kernel, devel, Network Development, lvs-devel,
netfilter-devel, coreteam, Ard Biesheuvel, Nishka Dasgupta
In-Reply-To: <20190628123819.2785504-3-arnd@arndb.de>
On Fri, Jun 28, 2019 at 8:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> An earlier patch I sent reduced the stack usage enough to get
> below the warning limit, and I could show this was safe, but with
> GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, it gets worse again because large stack
> variables in the same function no longer overlap:
>
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'translate_scan.isra.2':
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:322:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Split out the largest two blocks in the affected function into two
> separate functions and mark those noinline_for_stack.
>
> Fixes: 8c5af16f7953 ("staging: rtl8712: reduce stack usage")
> Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 5/9] libbpf: add tracepoint attach API
From: Song Liu @ 2019-06-28 19:50 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-6-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Allow attaching BPF programs to kernel tracepoint BPF hooks specified by
> category and name.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 78 ++++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 4 +++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 83 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 65d2fef41003..76d1599a7d56 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4229,6 +4229,84 @@ struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> return link;
> }
>
> +static int determine_tracepoint_id(const char* tp_category, const char* tp_name)
> +{
> + char file[PATH_MAX];
> + int ret;
> +
> + ret = snprintf(file, sizeof(file),
> + "/sys/kernel/debug/tracing/events/%s/%s/id",
> + tp_category, tp_name);
> + if (ret < 0)
> + return -errno;
> + if (ret >= sizeof(file)) {
> + pr_debug("tracepoint %s/%s path is too long\n",
> + tp_category, tp_name);
> + return -E2BIG;
> + }
> + return parse_uint_from_file(file);
> +}
> +
> +static int perf_event_open_tracepoint(const char* tp_category,
> + const char* tp_name)
> +{
> + struct perf_event_attr attr = {};
> + char errmsg[STRERR_BUFSIZE];
> + int tp_id, pfd, err;
> +
> + tp_id = determine_tracepoint_id(tp_category, tp_name);
> + if (tp_id < 0){
> + pr_warning("failed to determine tracepoint '%s/%s' perf ID: %s\n",
> + tp_category, tp_name,
> + libbpf_strerror_r(tp_id, errmsg, sizeof(errmsg)));
> + return tp_id;
> + }
> +
> + attr.type = PERF_TYPE_TRACEPOINT;
> + attr.size = sizeof(attr);
> + attr.config = tp_id;
> +
> + pfd = syscall( __NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + if (pfd < 0) {
> + err = -errno;
> + pr_warning("tracepoint '%s/%s' perf_event_open() failed: %s\n",
> + tp_category, tp_name,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return err;
> + }
Similar to the 4/9, I guess we can remove some duplicated pr_warning().
Thanks,
Song
> + return pfd;
> +}
> +
> +struct bpf_link *bpf_program__attach_tracepoint(struct bpf_program *prog,
> + const char *tp_category,
> + const char *tp_name)
> +{
> + char errmsg[STRERR_BUFSIZE];
> + struct bpf_link *link;
> + int pfd, err;
> +
> + pfd = perf_event_open_tracepoint(tp_category, tp_name);
> + if (pfd < 0) {
> + pr_warning("program '%s': failed to create tracepoint '%s/%s' perf event: %s\n",
> + bpf_program__title(prog, false),
> + tp_category, tp_name,
> + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> + return ERR_PTR(pfd);
> + }
> + link = bpf_program__attach_perf_event(prog, pfd);
> + if (IS_ERR(link)) {
> + close(pfd);
> + err = PTR_ERR(link);
> + pr_warning("program '%s': failed to attach to tracepoint '%s/%s': %s\n",
> + bpf_program__title(prog, false),
> + tp_category, tp_name,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return link;
> + }
> + return link;
> +}
> +
> enum bpf_perf_event_ret
> bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> void **copy_mem, size_t *copy_size,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index bd767cc11967..60611f4b4e1d 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -178,6 +178,10 @@ LIBBPF_API struct bpf_link *
> bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> pid_t pid, const char *binary_path,
> size_t func_offset);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_tracepoint(struct bpf_program *prog,
> + const char *tp_category,
> + const char *tp_name);
>
> struct bpf_insn;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 57a40fb60718..3c618b75ef65 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -171,6 +171,7 @@ LIBBPF_0.0.4 {
> bpf_object__load_xattr;
> bpf_program__attach_kprobe;
> bpf_program__attach_perf_event;
> + bpf_program__attach_tracepoint;
> bpf_program__attach_uprobe;
> btf_dump__dump_type;
> btf_dump__free;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Song Liu @ 2019-06-28 19:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-5-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Add ability to attach to kernel and user probes and retprobes.
> Implementation depends on perf event support for kprobes/uprobes.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 213 +++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 7 ++
> tools/lib/bpf/libbpf.map | 2 +
> 3 files changed, 222 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 606705f878ba..65d2fef41003 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4016,6 +4016,219 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> return (struct bpf_link *)link;
> }
>
> +static int parse_uint(const char *buf)
> +{
> + int ret;
> +
> + errno = 0;
> + ret = (int)strtol(buf, NULL, 10);
> + if (errno) {
> + ret = -errno;
> + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> + return ret;
> + }
> + if (ret < 0) {
> + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> + return -EINVAL;
> + }
> + return ret;
> +}
> +
> +static int parse_uint_from_file(const char* file)
> +{
> + char buf[STRERR_BUFSIZE];
> + int fd, ret;
> +
> + fd = open(file, O_RDONLY);
> + if (fd < 0) {
> + ret = -errno;
> + pr_debug("failed to open '%s': %s\n", file,
> + libbpf_strerror_r(ret, buf, sizeof(buf)));
> + return ret;
> + }
> + ret = read(fd, buf, sizeof(buf));
> + ret = ret < 0 ? -errno : ret;
> + close(fd);
> + if (ret < 0) {
> + pr_debug("failed to read '%s': %s\n", file,
> + libbpf_strerror_r(ret, buf, sizeof(buf)));
> + return ret;
> + }
> + if (ret == 0 || ret >= sizeof(buf)) {
> + buf[sizeof(buf) - 1] = 0;
> + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> + return -EINVAL;
> + }
> + return parse_uint(buf);
> +}
> +
> +static int determine_kprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> + return parse_uint_from_file(file);
> +}
> +
> +static int determine_uprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> + return parse_uint_from_file(file);
> +}
> +
> +static int parse_config_from_file(const char *file)
> +{
> + char buf[STRERR_BUFSIZE];
> + int fd, ret;
> +
> + fd = open(file, O_RDONLY);
> + if (fd < 0) {
> + ret = -errno;
> + pr_debug("failed to open '%s': %s\n", file,
> + libbpf_strerror_r(ret, buf, sizeof(buf)));
> + return ret;
> + }
> + ret = read(fd, buf, sizeof(buf));
> + ret = ret < 0 ? -errno : ret;
> + close(fd);
> + if (ret < 0) {
> + pr_debug("failed to read '%s': %s\n", file,
> + libbpf_strerror_r(ret, buf, sizeof(buf)));
> + return ret;
> + }
> + if (ret == 0 || ret >= sizeof(buf)) {
> + buf[sizeof(buf) - 1] = 0;
> + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> + return -EINVAL;
> + }
> + if (strncmp(buf, "config:", 7)) {
> + pr_debug("expected 'config:' prefix, found '%s'\n", buf);
> + return -EINVAL;
> + }
> + return parse_uint(buf + 7);
> +}
> +
> +static int determine_kprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> + return parse_config_from_file(file);
> +}
> +
> +static int determine_uprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> + return parse_config_from_file(file);
> +}
Can we do the above with fscanf? Would that be easier?
> +
> +static int perf_event_open_probe(bool uprobe, bool retprobe, const char* name,
> + uint64_t offset, int pid)
> +{
> + struct perf_event_attr attr = {};
> + char errmsg[STRERR_BUFSIZE];
> + int type, pfd, err;
> +
> + type = uprobe ? determine_uprobe_perf_type()
> + : determine_kprobe_perf_type();
> + if (type < 0) {
> + pr_warning("failed to determine %s perf type: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> + return type;
> + }
> + if (retprobe) {
> + int bit = uprobe ? determine_uprobe_retprobe_bit()
> + : determine_kprobe_retprobe_bit();
> +
> + if (bit < 0) {
> + pr_warning("failed to determine %s retprobe bit: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(bit, errmsg,
> + sizeof(errmsg)));
> + return bit;
> + }
> + attr.config |= 1 << bit;
> + }
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> +
> + /* pid filter is meaningful only for uprobes */
> + pfd = syscall(__NR_perf_event_open, &attr,
> + pid < 0 ? -1 : pid /* pid */,
> + pid == -1 ? 0 : -1 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + if (pfd < 0) {
> + err = -errno;
> + pr_warning("%s perf_event_open() failed: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
We have another warning in bpf_program__attach_[k|u]probe(). I guess
we can remove this one here.
> + return err;
> + }
> + return pfd;
> +}
> +
> +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> + bool retprobe,
> + const char *func_name)
> +{
> + char errmsg[STRERR_BUFSIZE];
> + struct bpf_link *link;
> + int pfd, err;
> +
> + pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> + 0 /* offset */, -1 /* pid */);
> + if (pfd < 0) {
> + pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "kretprobe" : "kprobe", func_name,
> + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> + return ERR_PTR(pfd);
> + }
> + link = bpf_program__attach_perf_event(prog, pfd);
> + if (IS_ERR(link)) {
> + close(pfd);
> + err = PTR_ERR(link);
> + pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "kretprobe" : "kprobe", func_name,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return link;
> + }
> + return link;
> +}
> +
> +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> + bool retprobe, pid_t pid,
> + const char *binary_path,
> + size_t func_offset)
> +{
> + char errmsg[STRERR_BUFSIZE];
> + struct bpf_link *link;
> + int pfd, err;
> +
> + pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> + binary_path, func_offset, pid);
> + if (pfd < 0) {
> + pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "uretprobe" : "uprobe",
> + binary_path, func_offset,
> + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> + return ERR_PTR(pfd);
> + }
> + link = bpf_program__attach_perf_event(prog, pfd);
> + if (IS_ERR(link)) {
> + close(pfd);
> + err = PTR_ERR(link);
> + pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "uretprobe" : "uprobe",
> + binary_path, func_offset,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return link;
> + }
> + return link;
> +}
> +
> enum bpf_perf_event_ret
> bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> void **copy_mem, size_t *copy_size,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1bf66c4a9330..bd767cc11967 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
>
> LIBBPF_API struct bpf_link *
> bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> + const char *func_name);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> + pid_t pid, const char *binary_path,
> + size_t func_offset);
>
> struct bpf_insn;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 756f5aa802e9..57a40fb60718 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
> global:
> bpf_link__destroy;
> bpf_object__load_xattr;
> + bpf_program__attach_kprobe;
> bpf_program__attach_perf_event;
> + bpf_program__attach_uprobe;
> btf_dump__dump_type;
> btf_dump__free;
> btf_dump__new;
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH v2 bpf-next] bpf: Add support for fq's EDT to HBM
From: brakmo @ 2019-06-28 19:41 UTC (permalink / raw)
To: netdev
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eric Dumazet, Kernel Team
Adds support for fq's Earliest Departure Time to HBM (Host Bandwidth
Manager). Includes a new BPF program supporting EDT, and also updates
corresponding programs.
It will drop packets with an EDT of more than 500us in the future
unless the packet belongs to a flow with less than 2 packets in flight.
This is done so each flow has at least 2 packets in flight, so they
will not starve, and also to help prevent delayed ACK timeouts.
It will also work with ECN enabled traffic, where the packets will be
CE marked if their EDT is more than 50us in the future.
The table below shows some performance numbers. The flows are back to
back RPCS. One server sending to another, either 2 or 4 flows.
One flow is a 10KB RPC, the rest are 1MB RPCs. When there are more
than one flow of a given RPC size, the numbers represent averages.
The rate limit applies to all flows (they are in the same cgroup).
Tests ending with "-edt" ran with the new BPF program supporting EDT.
Tests ending with "-hbt" ran on top HBT qdisc with the specified rate
(i.e. no HBM). The other tests ran with the HBM BPF program included
in the HBM patch-set.
EDT has limited value when using DCTCP, but it helps in many cases when
using Cubic. It usually achieves larger link utilization and lower
99% latencies for the 1MB RPCs.
HBM ends up queueing a lot of packets with its default parameter values,
reducing the goodput of the 10KB RPCs and increasing their latency. Also,
the RTTs seen by the flows are quite large.
Aggr 10K 10K 10K 1MB 1MB 1MB
Limit rate drops RTT rate P90 P99 rate P90 P99
Test rate Flows Mbps % us Mbps us us Mbps ms ms
-------- ---- ----- ---- ----- --- ---- ---- ---- ---- ---- ----
cubic 1G 2 904 0.02 108 257 511 539 647 13.4 24.5
cubic-edt 1G 2 982 0.01 156 239 656 967 743 14.0 17.2
dctcp 1G 2 977 0.00 105 324 408 744 653 14.5 15.9
dctcp-edt 1G 2 981 0.01 142 321 417 811 660 15.7 17.0
cubic-htb 1G 2 919 0.00 1825 40 2822 4140 879 9.7 9.9
cubic 200M 2 155 0.30 220 81 532 655 74 283 450
cubic-edt 200M 2 188 0.02 222 87 1035 1095 101 84 85
dctcp 200M 2 188 0.03 111 77 912 939 111 76 325
dctcp-edt 200M 2 188 0.03 217 74 1416 1738 114 76 79
cubic-htb 200M 2 188 0.00 5015 8 14ms 15ms 180 48 50
cubic 1G 4 952 0.03 110 165 516 546 262 38 154
cubic-edt 1G 4 973 0.01 190 111 1034 1314 287 65 79
dctcp 1G 4 951 0.00 103 180 617 905 257 37 38
dctcp-edt 1G 4 967 0.00 163 151 732 1126 272 43 55
cubic-htb 1G 4 914 0.00 3249 13 7ms 8ms 300 29 34
cubic 5G 4 4236 0.00 134 305 490 624 1310 10 17
cubic-edt 5G 4 4865 0.00 156 306 425 759 1520 10 16
dctcp 5G 4 4936 0.00 128 485 221 409 1484 7 9
dctcp-edt 5G 4 4924 0.00 148 390 392 623 1508 11 26
v1 -> v2: Incorporated Andrii's suggestions
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
samples/bpf/Makefile | 2 +
samples/bpf/do_hbm_test.sh | 22 ++---
samples/bpf/hbm.c | 18 +++-
samples/bpf/hbm_edt_kern.c | 171 +++++++++++++++++++++++++++++++++++++
samples/bpf/hbm_kern.h | 40 +++++++--
5 files changed, 234 insertions(+), 19 deletions(-)
create mode 100644 samples/bpf/hbm_edt_kern.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..35640414ebb3 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -168,6 +168,7 @@ always += task_fd_query_kern.o
always += xdp_sample_pkts_kern.o
always += ibumad_kern.o
always += hbm_out_kern.o
+always += hbm_edt_kern.o
KBUILD_HOSTCFLAGS += -I$(objtree)/usr/include
KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/bpf/
@@ -272,6 +273,7 @@ $(src)/*.c: verify_target_bpf $(LIBBPF)
$(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
$(obj)/hbm_out_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
$(obj)/hbm.o: $(src)/hbm.h
+$(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
# asm/sysreg.h - inline assembly used by it is incompatible with llvm.
# But, there is no easy way to fix it, so just exclude it since it is
diff --git a/samples/bpf/do_hbm_test.sh b/samples/bpf/do_hbm_test.sh
index e48b047d4646..ffe4c0607341 100755
--- a/samples/bpf/do_hbm_test.sh
+++ b/samples/bpf/do_hbm_test.sh
@@ -14,7 +14,7 @@ Usage() {
echo "loads. The output is the goodput in Mbps (unless -D was used)."
echo ""
echo "USAGE: $name [out] [-b=<prog>|--bpf=<prog>] [-c=<cc>|--cc=<cc>]"
- echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E]"
+ echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E] [--edt]"
echo " [-f=<#flows>|--flows=<#flows>] [-h] [-i=<id>|--id=<id >]"
echo " [-l] [-N] [--no_cn] [-p=<port>|--port=<port>] [-P]"
echo " [-q=<qdisc>] [-R] [-s=<server>|--server=<server]"
@@ -30,6 +30,7 @@ Usage() {
echo " other detailed information. This information is"
echo " test dependent (i.e. iperf3 or netperf)."
echo " -E enable ECN (not required for dctcp)"
+ echo " --edt use fq's Earliest Departure Time (requires fq)"
echo " -f or --flows number of concurrent flows (default=1)"
echo " -i or --id cgroup id (an integer, default is 1)"
echo " -N use netperf instead of iperf3"
@@ -130,13 +131,12 @@ processArgs () {
details=1
;;
-E)
- ecn=1
+ ecn=1
+ ;;
+ --edt)
+ flags="$flags --edt"
+ qdisc="fq"
;;
- # Support for upcomming fq Early Departure Time egress rate limiting
- #--edt)
- # prog="hbm_out_edt_kern.o"
- # qdisc="fq"
- # ;;
-f=*|--flows=*)
flows="${i#*=}"
;;
@@ -228,8 +228,8 @@ if [ "$netem" -ne "0" ] ; then
tc qdisc del dev lo root > /dev/null 2>&1
tc qdisc add dev lo root netem delay $netem\ms > /dev/null 2>&1
elif [ "$qdisc" != "" ] ; then
- tc qdisc del dev lo root > /dev/null 2>&1
- tc qdisc add dev lo root $qdisc > /dev/null 2>&1
+ tc qdisc del dev eth0 root > /dev/null 2>&1
+ tc qdisc add dev eth0 root $qdisc > /dev/null 2>&1
fi
n=0
@@ -399,7 +399,9 @@ fi
if [ "$netem" -ne "0" ] ; then
tc qdisc del dev lo root > /dev/null 2>&1
fi
-
+if [ "$qdisc" != "" ] ; then
+ tc qdisc del dev eth0 root > /dev/null 2>&1
+fi
sleep 2
hbmPid=`ps ax | grep "hbm " | grep --invert-match "grep" | awk '{ print $1 }'`
diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c
index b905b32ff185..e0fbab9bec83 100644
--- a/samples/bpf/hbm.c
+++ b/samples/bpf/hbm.c
@@ -62,6 +62,7 @@ bool loopback_flag;
bool debugFlag;
bool work_conserving_flag;
bool no_cn_flag;
+bool edt_flag;
static void Usage(void);
static void read_trace_pipe2(void);
@@ -372,9 +373,14 @@ static int run_bpf_prog(char *prog, int cg_id)
fprintf(fout, "avg rtt:%d\n",
(int)(qstats.sum_rtt / (qstats.pkts_total + 1)));
// Average credit
- fprintf(fout, "avg credit:%d\n",
- (int)(qstats.sum_credit /
- (1500 * ((int)qstats.pkts_total) + 1)));
+ if (edt_flag)
+ fprintf(fout, "avg credit_ms:%.03f\n",
+ (qstats.sum_credit /
+ (qstats.pkts_total + 1.0)) / 1000000.0);
+ else
+ fprintf(fout, "avg credit:%d\n",
+ (int)(qstats.sum_credit /
+ (1500 * ((int)qstats.pkts_total ) + 1)));
// Return values stats
for (k = 0; k < RET_VAL_COUNT; k++) {
@@ -408,6 +414,7 @@ static void Usage(void)
" Where:\n"
" -o indicates egress direction (default)\n"
" -d print BPF trace debug buffer\n"
+ " --edt use fq's Earliest Departure Time\n"
" -l also limit flows using loopback\n"
" -n <#> to create cgroup \"/hbm#\" and attach prog\n"
" Default is /hbm1\n"
@@ -433,6 +440,7 @@ int main(int argc, char **argv)
char *optstring = "iodln:r:st:wh";
struct option loptions[] = {
{"no_cn", 0, NULL, 1},
+ {"edt", 0, NULL, 2},
{NULL, 0, NULL, 0}
};
@@ -441,6 +449,10 @@ int main(int argc, char **argv)
case 1:
no_cn_flag = true;
break;
+ case 2:
+ prog = "hbm_edt_kern.o";
+ edt_flag = true;
+ break;
case'o':
break;
case 'd':
diff --git a/samples/bpf/hbm_edt_kern.c b/samples/bpf/hbm_edt_kern.c
new file mode 100644
index 000000000000..004a44a83e1e
--- /dev/null
+++ b/samples/bpf/hbm_edt_kern.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019 Facebook
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * Sample Host Bandwidth Manager (HBM) BPF program.
+ *
+ * A cgroup skb BPF egress program to limit cgroup output bandwidth.
+ * It uses a modified virtual token bucket queue to limit average
+ * egress bandwidth. The implementation uses credits instead of tokens.
+ * Negative credits imply that queueing would have happened (this is
+ * a virtual queue, so no queueing is done by it. However, queueing may
+ * occur at the actual qdisc (which is not used for rate limiting).
+ *
+ * This implementation uses 3 thresholds, one to start marking packets and
+ * the other two to drop packets:
+ * CREDIT
+ * - <--------------------------|------------------------> +
+ * | | | 0
+ * | Large pkt |
+ * | drop thresh |
+ * Small pkt drop Mark threshold
+ * thresh
+ *
+ * The effect of marking depends on the type of packet:
+ * a) If the packet is ECN enabled and it is a TCP packet, then the packet
+ * is ECN marked.
+ * b) If the packet is a TCP packet, then we probabilistically call tcp_cwr
+ * to reduce the congestion window. The current implementation uses a linear
+ * distribution (0% probability at marking threshold, 100% probability
+ * at drop threshold).
+ * c) If the packet is not a TCP packet, then it is dropped.
+ *
+ * If the credit is below the drop threshold, the packet is dropped. If it
+ * is a TCP packet, then it also calls tcp_cwr since packets dropped by
+ * by a cgroup skb BPF program do not automatically trigger a call to
+ * tcp_cwr in the current kernel code.
+ *
+ * This BPF program actually uses 2 drop thresholds, one threshold
+ * for larger packets (>= 120 bytes) and another for smaller packets. This
+ * protects smaller packets such as SYNs, ACKs, etc.
+ *
+ * The default bandwidth limit is set at 1Gbps but this can be changed by
+ * a user program through a shared BPF map. In addition, by default this BPF
+ * program does not limit connections using loopback. This behavior can be
+ * overwritten by the user program. There is also an option to calculate
+ * some statistics, such as percent of packets marked or dropped, which
+ * a user program, such as hbm, can access.
+ */
+
+#include "hbm_kern.h"
+
+SEC("cgroup_skb/egress")
+int _hbm_out_cg(struct __sk_buff *skb)
+{
+ signed long long delta = 0, delta_send;
+ unsigned long long curtime, sendtime;
+ struct hbm_queue_stats *qsp = NULL;
+ unsigned int queue_index = 0;
+ bool congestion_flag = false;
+ bool ecn_ce_flag = false;
+ struct hbm_pkt_info pkti;
+ struct hbm_vqueue *qdp;
+ bool drop_flag = false;
+ bool cwr_flag = false;
+ int len = skb->len;
+ int rv = ALLOW_PKT;
+
+ qsp = bpf_map_lookup_elem(&queue_stats, &queue_index);
+ if (qsp != NULL && !qsp->loopback && (skb->ifindex == 1))
+ return ALLOW_PKT;
+
+ hbm_get_pkt_info(skb, &pkti);
+
+ // We may want to account for the length of headers in len
+ // calculation, like ETH header + overhead, specially if it
+ // is a gso packet. But I am not doing it right now.
+
+ qdp = bpf_get_local_storage(&queue_state, 0);
+ if (!qdp)
+ return ALLOW_PKT;
+ if (qdp->lasttime == 0)
+ hbm_init_edt_vqueue(qdp, 1024);
+
+ curtime = bpf_ktime_get_ns();
+
+ // Begin critical section
+ bpf_spin_lock(&qdp->lock);
+ delta = qdp->lasttime - curtime;
+ // bound bursts to 100us
+ if (delta < -BURST_SIZE_NS) {
+ // negative delta is a credit that allows bursts
+ qdp->lasttime = curtime - BURST_SIZE_NS;
+ delta = -BURST_SIZE_NS;
+ }
+ sendtime = qdp->lasttime;
+ delta_send = BYTES_TO_NS(len, qdp->rate);
+ qdp->lasttime += delta_send;
+ bpf_spin_unlock(&qdp->lock);
+ // End critical section
+
+ // Set EDT of packet
+ skb->tstamp = sendtime;
+
+ // Check if we should update rate
+ if (qsp != NULL && (qsp->rate * 128) != qdp->rate) {
+ qdp->rate = qsp->rate * 128;
+ bpf_printk("Updating rate: %d (1sec:%llu bits)\n",
+ (int)qdp->rate,
+ CREDIT_PER_NS(1000000000, qdp->rate) * 8);
+ }
+
+ // Set flags (drop, congestion, cwr)
+ // last packet will be sent in the future, bound latency
+ if (delta > DROP_THRESH_NS || (delta > LARGE_PKT_DROP_THRESH_NS &&
+ len > LARGE_PKT_THRESH)) {
+ drop_flag = true;
+ if (pkti.is_tcp && pkti.ecn == 0)
+ cwr_flag = true;
+ } else if (delta > MARK_THRESH_NS) {
+ if (pkti.is_tcp)
+ congestion_flag = true;
+ else
+ drop_flag = true;
+ }
+
+ if (congestion_flag) {
+ if (bpf_skb_ecn_set_ce(skb)) {
+ ecn_ce_flag = true;
+ } else {
+ if (pkti.is_tcp) {
+ unsigned int rand = bpf_get_prandom_u32();
+
+ if (delta >= MARK_THRESH_NS +
+ (rand % MARK_REGION_SIZE_NS)) {
+ // Do congestion control
+ cwr_flag = true;
+ }
+ } else if (len > LARGE_PKT_THRESH) {
+ // Problem if too many small packets?
+ drop_flag = true;
+ congestion_flag = false;
+ }
+ }
+ }
+
+ if (pkti.is_tcp && drop_flag && pkti.packets_out <= 1) {
+ drop_flag = false;
+ cwr_flag = true;
+ congestion_flag = false;
+ }
+
+ if (qsp != NULL && qsp->no_cn)
+ cwr_flag = false;
+
+ hbm_update_stats(qsp, len, curtime, congestion_flag, drop_flag,
+ cwr_flag, ecn_ce_flag, &pkti, (int) delta);
+
+ if (drop_flag) {
+ __sync_add_and_fetch(&(qdp->credit), len);
+ __sync_add_and_fetch(&(qdp->lasttime), -delta_send);
+ rv = DROP_PKT;
+ }
+
+ if (cwr_flag)
+ rv |= CWR;
+ return rv;
+}
+char _license[] SEC("license") = "GPL";
diff --git a/samples/bpf/hbm_kern.h b/samples/bpf/hbm_kern.h
index be19cf1d5cd5..aa207a2eebbd 100644
--- a/samples/bpf/hbm_kern.h
+++ b/samples/bpf/hbm_kern.h
@@ -29,6 +29,7 @@
#define DROP_PKT 0
#define ALLOW_PKT 1
#define TCP_ECN_OK 1
+#define CWR 2
#ifndef HBM_DEBUG // Define HBM_DEBUG to enable debugging
#undef bpf_printk
@@ -45,8 +46,18 @@
#define MAX_CREDIT (100 * MAX_BYTES_PER_PACKET)
#define INIT_CREDIT (INITIAL_CREDIT_PACKETS * MAX_BYTES_PER_PACKET)
+// Time base accounting for fq's EDT
+#define BURST_SIZE_NS 100000 // 100us
+#define MARK_THRESH_NS 50000 // 50us
+#define DROP_THRESH_NS 500000 // 500us
+// Reserve 20us of queuing for small packets (less than 120 bytes)
+#define LARGE_PKT_DROP_THRESH_NS (DROP_THRESH_NS - 20000)
+#define MARK_REGION_SIZE_NS (LARGE_PKT_DROP_THRESH_NS - MARK_THRESH_NS)
+
// rate in bytes per ns << 20
#define CREDIT_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
+#define BYTES_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
+#define BYTES_TO_NS(bytes, rate) div64_u64(((u64)(bytes)) << 20, (u64)(rate))
struct bpf_map_def SEC("maps") queue_state = {
.type = BPF_MAP_TYPE_CGROUP_STORAGE,
@@ -67,6 +78,7 @@ BPF_ANNOTATE_KV_PAIR(queue_stats, int, struct hbm_queue_stats);
struct hbm_pkt_info {
int cwnd;
int rtt;
+ int packets_out;
bool is_ip;
bool is_tcp;
short ecn;
@@ -86,16 +98,20 @@ static int get_tcp_info(struct __sk_buff *skb, struct hbm_pkt_info *pkti)
if (tp) {
pkti->cwnd = tp->snd_cwnd;
pkti->rtt = tp->srtt_us >> 3;
+ pkti->packets_out = tp->packets_out;
return 0;
}
}
}
}
+ pkti->cwnd = 0;
+ pkti->rtt = 0;
+ pkti->packets_out = 0;
return 1;
}
-static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
- struct hbm_pkt_info *pkti)
+static void hbm_get_pkt_info(struct __sk_buff *skb,
+ struct hbm_pkt_info *pkti)
{
struct iphdr iph;
struct ipv6hdr *ip6h;
@@ -123,10 +139,22 @@ static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
static __always_inline void hbm_init_vqueue(struct hbm_vqueue *qdp, int rate)
{
- bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
- qdp->lasttime = bpf_ktime_get_ns();
- qdp->credit = INIT_CREDIT;
- qdp->rate = rate * 128;
+ bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
+ qdp->lasttime = bpf_ktime_get_ns();
+ qdp->credit = INIT_CREDIT;
+ qdp->rate = rate * 128;
+}
+
+static __always_inline void hbm_init_edt_vqueue(struct hbm_vqueue *qdp,
+ int rate)
+{
+ unsigned long long curtime;
+
+ curtime = bpf_ktime_get_ns();
+ bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
+ qdp->lasttime = curtime - BURST_SIZE_NS; // support initial burst
+ qdp->credit = 0; // not used
+ qdp->rate = rate * 128;
}
static __always_inline void hbm_update_stats(struct hbm_queue_stats *qsp,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/2] tls: remove close callback sock unlock/lock and flush_sync
From: John Fastabend @ 2019-06-28 19:40 UTC (permalink / raw)
To: Jakub Kicinski, John Fastabend; +Cc: daniel, ast, netdev, edumazet, bpf
In-Reply-To: <20190628113100.597bfbe6@cakuba.netronome.com>
Jakub Kicinski wrote:
> On Fri, 28 Jun 2019 07:12:07 -0700, John Fastabend wrote:
> > Yeah seems possible although never seen in my testing. So I'll
> > move the test_bit() inside the lock and do a ctx check to ensure
> > still have the reference.
> >
> > CPU 0 (free) CPU 1 (wq)
> >
> > lock(sk)
> > lock(sk)
> > set_bit()
> > cancel_work()
> > release
> > ctx = tls_get_ctx(sk)
> > unlikely(!ctx) <- we may have free'd
> > test_bit()
> > ...
> > release()
> >
> > or
> >
> > CPU 0 (free) CPU 1 (wq)
> >
> > lock(sk)
> > lock(sk)
> > ctx = tls_get_ctx(sk)
> > unlikely(!ctx)
> > test_bit()
> > ...
> > release()
> > set_bit()
> > cancel_work()
> > release
>
> Hmm... perhaps it's cleanest to stop the work from scheduling before we
> proceed?
>
> close():
> while (!test_and_set(SHED))
> flush();
>
> lock(sk);
> ...
>
> We just need to move init work, no?
The lock() is already held when entering unhash() side so need to
handle this case as well,
CPU 0 (free) CPU 1 (wq)
lock(sk) ctx = tls_get_ctx(sk) <- need to be check null ptr
sk_prot->unhash()
set_bit()
cancel_work()
...
kfree(ctx)
unlock(sk)
but using cancel and doing an unlikely(!ctx) check should be
sufficient to handle wq. What I'm not sure how to solve now is
in patch 2 of this series unhash is still calling strp_done
with the sock lock. Maybe we need to do a deferred release
like sockmap side?
Trying to drop the lock and then grabbing it again doesn't
seem right to me seems based on comment in tcp_abort we
could potentially "race with userspace socket closes such
as tcp_close". iirc I think one of the tls splats from syzbot
looked something like this may have happened.
For now I'm considering adding a strp_cancel() op. Seeing
we are closing() the socket and tearkng down we can probably
be OK with throwing out strp results.
>
> FWIW I never tested his async crypto stuff, I wonder if there is a way
> to convince normal CPU crypto to pretend to be async?
^ permalink raw reply
* Re: BUG: unable to handle kernel paging request in hrtimer_interrupt
From: syzbot @ 2019-06-28 19:39 UTC (permalink / raw)
To: ast, bpf, daniel, dvyukov, hdanton, john.fastabend, linux-kernel,
netdev, syzkaller-bugs, tglx
In-Reply-To: <0000000000001c03bf058baf488a@google.com>
syzbot has bisected this bug to:
commit e9db4ef6bf4ca9894bb324c76e01b8f1a16b2650
Author: John Fastabend <john.fastabend@gmail.com>
Date: Sat Jun 30 13:17:47 2018 +0000
bpf: sockhash fix omitted bucket lock in sock_close
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14436833a00000
start commit: 29f785ff Merge branch 'fixes' of git://git.kernel.org/pub/..
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=16436833a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=12436833a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=e5c77f8090a3b96b
dashboard link: https://syzkaller.appspot.com/bug?extid=037e18398ba8c655a652
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16da8cc9a00000
Reported-by: syzbot+037e18398ba8c655a652@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH v3 bpf-next 2/9] libbpf: introduce concept of bpf_link
From: Song Liu @ 2019-06-28 19:37 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-3-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> bpf_link is and abstraction of an association of a BPF program and one
> of many possible BPF attachment points (hooks). This allows to have
> uniform interface for detaching BPF programs regardless of the nature of
> link and how it was created. Details of creation and setting up of
> a specific bpf_link is handled by corresponding attachment methods
> (bpf_program__attach_xxx) added in subsequent commits. Once successfully
> created, bpf_link has to be eventually destroyed with
> bpf_link__destroy(), at which point BPF program is disassociated from
> a hook and all the relevant resources are freed.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: fix precision tracking
From: Lawrence Brakmo @ 2019-06-28 19:36 UTC (permalink / raw)
To: Andrii Nakryiko, Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Networking, bpf, Kernel Team
In-Reply-To: <CAEf4BzY00WVr452Pj1JDMSG4nD47uexp+G+zHZxijZmSS1bUKw@mail.gmail.com>
On 6/28/19, 12:33 PM, "netdev-owner@vger.kernel.org on behalf of Andrii Nakryiko" <netdev-owner@vger.kernel.org on behalf of andrii.nakryiko@gmail.com> wrote:
On Fri, Jun 28, 2019 at 9:25 AM Alexei Starovoitov <ast@kernel.org> wrote:
>
> When equivalent state is found the current state needs to propagate precision marks.
> Otherwise the verifier will prune the search incorrectly.
>
> There is a price for correctness:
> before before broken fixed
> cnst spill precise precise
> bpf_lb-DLB_L3.o 1923 8128 1863 1898
> bpf_lb-DLB_L4.o 3077 6707 2468 2666
> bpf_lb-DUNKNOWN.o 1062 1062 544 544
> bpf_lxc-DDROP_ALL.o 166729 380712 22629 36823
> bpf_lxc-DUNKNOWN.o 174607 440652 28805 45325
> bpf_netdev.o 8407 31904 6801 7002
> bpf_overlay.o 5420 23569 4754 4858
> bpf_lxc_jit.o 39389 359445 50925 69631
> Overall precision tracking is still very effective.
>
> Fixes: b5dc0163d8fd ("bpf: precise scalar_value tracking")
> Reported-by: Lawrence Brakmo <brakmo@fb.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
Tested-by: Lawrence Brakmo <brakmo@fb.com>
> Sending the fix early w/o tests, since I'm traveling.
> Will add proper tests when I'm back.
> ---
> kernel/bpf/verifier.c | 121 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 107 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6b5623d320f9..62afc4058ced 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1659,16 +1659,18 @@ static void mark_all_scalars_precise(struct bpf_verifier_env *env,
> }
> }
>
> -static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> +static int __mark_chain_precision(struct bpf_verifier_env *env, int regno,
> + int spi)
> {
> struct bpf_verifier_state *st = env->cur_state;
> int first_idx = st->first_insn_idx;
> int last_idx = env->insn_idx;
> struct bpf_func_state *func;
> struct bpf_reg_state *reg;
> - u32 reg_mask = 1u << regno;
> - u64 stack_mask = 0;
> + u32 reg_mask = regno >= 0 ? 1u << regno : 0;
> + u64 stack_mask = spi >= 0 ? 1ull << spi : 0;
> bool skip_first = true;
> + bool new_marks = false;
> int i, err;
>
> if (!env->allow_ptr_leaks)
> @@ -1676,18 +1678,43 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return 0;
>
> func = st->frame[st->curframe];
> - reg = &func->regs[regno];
> - if (reg->type != SCALAR_VALUE) {
> - WARN_ONCE(1, "backtracing misuse");
> - return -EFAULT;
> + if (regno >= 0) {
> + reg = &func->regs[regno];
> + if (reg->type != SCALAR_VALUE) {
> + WARN_ONCE(1, "backtracing misuse");
> + return -EFAULT;
> + }
> + if (!reg->precise)
> + new_marks = true;
> + else
> + reg_mask = 0;
> + reg->precise = true;
> }
> - if (reg->precise)
> - return 0;
> - func->regs[regno].precise = true;
>
> + while (spi >= 0) {
> + if (func->stack[spi].slot_type[0] != STACK_SPILL) {
> + stack_mask = 0;
> + break;
> + }
> + reg = &func->stack[spi].spilled_ptr;
> + if (reg->type != SCALAR_VALUE) {
> + stack_mask = 0;
> + break;
> + }
> + if (!reg->precise)
> + new_marks = true;
> + else
> + stack_mask = 0;
> + reg->precise = true;
> + break;
> + }
> +
> + if (!new_marks)
> + return 0;
> + if (!reg_mask && !stack_mask)
> + return 0;
> for (;;) {
> DECLARE_BITMAP(mask, 64);
> - bool new_marks = false;
> u32 history = st->jmp_history_cnt;
>
> if (env->log.level & BPF_LOG_LEVEL)
> @@ -1730,12 +1757,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> if (!st)
> break;
>
> + new_marks = false;
> func = st->frame[st->curframe];
> bitmap_from_u64(mask, reg_mask);
> for_each_set_bit(i, mask, 32) {
> reg = &func->regs[i];
> - if (reg->type != SCALAR_VALUE)
> + if (reg->type != SCALAR_VALUE) {
> + reg_mask &= ~(1u << i);
> continue;
> + }
> if (!reg->precise)
> new_marks = true;
> reg->precise = true;
> @@ -1756,11 +1786,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return -EFAULT;
> }
>
> - if (func->stack[i].slot_type[0] != STACK_SPILL)
> + if (func->stack[i].slot_type[0] != STACK_SPILL) {
> + stack_mask &= ~(1ull << i);
> continue;
> + }
> reg = &func->stack[i].spilled_ptr;
> - if (reg->type != SCALAR_VALUE)
> + if (reg->type != SCALAR_VALUE) {
> + stack_mask &= ~(1ull << i);
> continue;
> + }
> if (!reg->precise)
> new_marks = true;
> reg->precise = true;
> @@ -1772,6 +1806,8 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> reg_mask, stack_mask);
> }
>
> + if (!reg_mask && !stack_mask)
> + break;
> if (!new_marks)
> break;
>
> @@ -1781,6 +1817,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return 0;
> }
>
> +static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> +{
> + return __mark_chain_precision(env, regno, -1);
> +}
> +
> +static int mark_chain_precision_stack(struct bpf_verifier_env *env, int spi)
> +{
> + return __mark_chain_precision(env, -1, spi);
> +}
>
> static bool is_spillable_regtype(enum bpf_reg_type type)
> {
> @@ -7114,6 +7159,46 @@ static int propagate_liveness(struct bpf_verifier_env *env,
> return 0;
> }
>
> +/* find precise scalars in the previous equivalent state and
> + * propagate them into the current state
> + */
> +static int propagate_precision(struct bpf_verifier_env *env,
> + const struct bpf_verifier_state *old)
> +{
> + struct bpf_reg_state *state_reg;
> + struct bpf_func_state *state;
> + int i, err = 0;
> +
> + state = old->frame[old->curframe];
> + state_reg = state->regs;
> + for (i = 0; i < BPF_REG_FP; i++, state_reg++) {
> + if (state_reg->type != SCALAR_VALUE ||
> + !state_reg->precise)
> + continue;
> + if (env->log.level & BPF_LOG_LEVEL2)
> + verbose(env, "propagating r%d\n", i);
> + err = mark_chain_precision(env, i);
> + if (err < 0)
> + return err;
> + }
> +
> + for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> + if (state->stack[i].slot_type[0] != STACK_SPILL)
> + continue;
> + state_reg = &state->stack[i].spilled_ptr;
> + if (state_reg->type != SCALAR_VALUE ||
> + !state_reg->precise)
> + continue;
> + if (env->log.level & BPF_LOG_LEVEL2)
> + verbose(env, "propagating fp%d\n",
> + (-i - 1) * BPF_REG_SIZE);
> + err = mark_chain_precision_stack(env, i);
> + if (err < 0)
> + return err;
> + }
> + return 0;
> +}
> +
> static bool states_maybe_looping(struct bpf_verifier_state *old,
> struct bpf_verifier_state *cur)
> {
> @@ -7206,6 +7291,14 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
> * this state and will pop a new one.
> */
> err = propagate_liveness(env, &sl->state, cur);
> +
> + /* if previous state reached the exit with precision and
> + * current state is equivalent to it (except precsion marks)
> + * the precision needs to be propagated back in
> + * the current state.
> + */
> + err = err ? : push_jmp_history(env, cur);
> + err = err ? : propagate_precision(env, &sl->state);
> if (err)
> return err;
> return 1;
> --
> 2.20.0
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 1/9] libbpf: make libbpf_strerror_r agnostic to sign of error
From: Song Liu @ 2019-06-28 19:34 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
bpf, Stanislav Fomichev, Kernel Team
In-Reply-To: <20190628055303.1249758-2-andriin@fb.com>
On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> It's often inconvenient to switch sign of error when passing it into
> libbpf_strerror_r. It's better for it to handle that automatically.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/lib/bpf/str_error.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/str_error.c b/tools/lib/bpf/str_error.c
> index 00e48ac5b806..b8064eedc177 100644
> --- a/tools/lib/bpf/str_error.c
> +++ b/tools/lib/bpf/str_error.c
> @@ -11,7 +11,7 @@
> */
> char *libbpf_strerror_r(int err, char *dst, int len)
> {
> - int ret = strerror_r(err, dst, len);
> + int ret = strerror_r(err < 0 ? -err : err, dst, len);
> if (ret)
> snprintf(dst, len, "ERROR: strerror_r(%d)=%d", err, ret);
> return dst;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: fix precision tracking
From: Andrii Nakryiko @ 2019-06-28 19:33 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Networking, bpf, Kernel Team
In-Reply-To: <20190628162409.2513499-1-ast@kernel.org>
On Fri, Jun 28, 2019 at 9:25 AM Alexei Starovoitov <ast@kernel.org> wrote:
>
> When equivalent state is found the current state needs to propagate precision marks.
> Otherwise the verifier will prune the search incorrectly.
>
> There is a price for correctness:
> before before broken fixed
> cnst spill precise precise
> bpf_lb-DLB_L3.o 1923 8128 1863 1898
> bpf_lb-DLB_L4.o 3077 6707 2468 2666
> bpf_lb-DUNKNOWN.o 1062 1062 544 544
> bpf_lxc-DDROP_ALL.o 166729 380712 22629 36823
> bpf_lxc-DUNKNOWN.o 174607 440652 28805 45325
> bpf_netdev.o 8407 31904 6801 7002
> bpf_overlay.o 5420 23569 4754 4858
> bpf_lxc_jit.o 39389 359445 50925 69631
> Overall precision tracking is still very effective.
>
> Fixes: b5dc0163d8fd ("bpf: precise scalar_value tracking")
> Reported-by: Lawrence Brakmo <brakmo@fb.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> Sending the fix early w/o tests, since I'm traveling.
> Will add proper tests when I'm back.
> ---
> kernel/bpf/verifier.c | 121 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 107 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6b5623d320f9..62afc4058ced 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1659,16 +1659,18 @@ static void mark_all_scalars_precise(struct bpf_verifier_env *env,
> }
> }
>
> -static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> +static int __mark_chain_precision(struct bpf_verifier_env *env, int regno,
> + int spi)
> {
> struct bpf_verifier_state *st = env->cur_state;
> int first_idx = st->first_insn_idx;
> int last_idx = env->insn_idx;
> struct bpf_func_state *func;
> struct bpf_reg_state *reg;
> - u32 reg_mask = 1u << regno;
> - u64 stack_mask = 0;
> + u32 reg_mask = regno >= 0 ? 1u << regno : 0;
> + u64 stack_mask = spi >= 0 ? 1ull << spi : 0;
> bool skip_first = true;
> + bool new_marks = false;
> int i, err;
>
> if (!env->allow_ptr_leaks)
> @@ -1676,18 +1678,43 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return 0;
>
> func = st->frame[st->curframe];
> - reg = &func->regs[regno];
> - if (reg->type != SCALAR_VALUE) {
> - WARN_ONCE(1, "backtracing misuse");
> - return -EFAULT;
> + if (regno >= 0) {
> + reg = &func->regs[regno];
> + if (reg->type != SCALAR_VALUE) {
> + WARN_ONCE(1, "backtracing misuse");
> + return -EFAULT;
> + }
> + if (!reg->precise)
> + new_marks = true;
> + else
> + reg_mask = 0;
> + reg->precise = true;
> }
> - if (reg->precise)
> - return 0;
> - func->regs[regno].precise = true;
>
> + while (spi >= 0) {
> + if (func->stack[spi].slot_type[0] != STACK_SPILL) {
> + stack_mask = 0;
> + break;
> + }
> + reg = &func->stack[spi].spilled_ptr;
> + if (reg->type != SCALAR_VALUE) {
> + stack_mask = 0;
> + break;
> + }
> + if (!reg->precise)
> + new_marks = true;
> + else
> + stack_mask = 0;
> + reg->precise = true;
> + break;
> + }
> +
> + if (!new_marks)
> + return 0;
> + if (!reg_mask && !stack_mask)
> + return 0;
> for (;;) {
> DECLARE_BITMAP(mask, 64);
> - bool new_marks = false;
> u32 history = st->jmp_history_cnt;
>
> if (env->log.level & BPF_LOG_LEVEL)
> @@ -1730,12 +1757,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> if (!st)
> break;
>
> + new_marks = false;
> func = st->frame[st->curframe];
> bitmap_from_u64(mask, reg_mask);
> for_each_set_bit(i, mask, 32) {
> reg = &func->regs[i];
> - if (reg->type != SCALAR_VALUE)
> + if (reg->type != SCALAR_VALUE) {
> + reg_mask &= ~(1u << i);
> continue;
> + }
> if (!reg->precise)
> new_marks = true;
> reg->precise = true;
> @@ -1756,11 +1786,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return -EFAULT;
> }
>
> - if (func->stack[i].slot_type[0] != STACK_SPILL)
> + if (func->stack[i].slot_type[0] != STACK_SPILL) {
> + stack_mask &= ~(1ull << i);
> continue;
> + }
> reg = &func->stack[i].spilled_ptr;
> - if (reg->type != SCALAR_VALUE)
> + if (reg->type != SCALAR_VALUE) {
> + stack_mask &= ~(1ull << i);
> continue;
> + }
> if (!reg->precise)
> new_marks = true;
> reg->precise = true;
> @@ -1772,6 +1806,8 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> reg_mask, stack_mask);
> }
>
> + if (!reg_mask && !stack_mask)
> + break;
> if (!new_marks)
> break;
>
> @@ -1781,6 +1817,15 @@ static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> return 0;
> }
>
> +static int mark_chain_precision(struct bpf_verifier_env *env, int regno)
> +{
> + return __mark_chain_precision(env, regno, -1);
> +}
> +
> +static int mark_chain_precision_stack(struct bpf_verifier_env *env, int spi)
> +{
> + return __mark_chain_precision(env, -1, spi);
> +}
>
> static bool is_spillable_regtype(enum bpf_reg_type type)
> {
> @@ -7114,6 +7159,46 @@ static int propagate_liveness(struct bpf_verifier_env *env,
> return 0;
> }
>
> +/* find precise scalars in the previous equivalent state and
> + * propagate them into the current state
> + */
> +static int propagate_precision(struct bpf_verifier_env *env,
> + const struct bpf_verifier_state *old)
> +{
> + struct bpf_reg_state *state_reg;
> + struct bpf_func_state *state;
> + int i, err = 0;
> +
> + state = old->frame[old->curframe];
> + state_reg = state->regs;
> + for (i = 0; i < BPF_REG_FP; i++, state_reg++) {
> + if (state_reg->type != SCALAR_VALUE ||
> + !state_reg->precise)
> + continue;
> + if (env->log.level & BPF_LOG_LEVEL2)
> + verbose(env, "propagating r%d\n", i);
> + err = mark_chain_precision(env, i);
> + if (err < 0)
> + return err;
> + }
> +
> + for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> + if (state->stack[i].slot_type[0] != STACK_SPILL)
> + continue;
> + state_reg = &state->stack[i].spilled_ptr;
> + if (state_reg->type != SCALAR_VALUE ||
> + !state_reg->precise)
> + continue;
> + if (env->log.level & BPF_LOG_LEVEL2)
> + verbose(env, "propagating fp%d\n",
> + (-i - 1) * BPF_REG_SIZE);
> + err = mark_chain_precision_stack(env, i);
> + if (err < 0)
> + return err;
> + }
> + return 0;
> +}
> +
> static bool states_maybe_looping(struct bpf_verifier_state *old,
> struct bpf_verifier_state *cur)
> {
> @@ -7206,6 +7291,14 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
> * this state and will pop a new one.
> */
> err = propagate_liveness(env, &sl->state, cur);
> +
> + /* if previous state reached the exit with precision and
> + * current state is equivalent to it (except precsion marks)
> + * the precision needs to be propagated back in
> + * the current state.
> + */
> + err = err ? : push_jmp_history(env, cur);
> + err = err ? : propagate_precision(env, &sl->state);
> if (err)
> return err;
> return 1;
> --
> 2.20.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox