[bpf-next,v4,0/3] Add skb + xdp dynptrs

Message

Joanne Koong Aug. 22, 2022, 11:56 p.m. UTC

This patchset is the 2nd in the dynptr series. The 1st can be found here [0].

This patchset adds skb and xdp type dynptrs, which have two main benefits for
packet parsing:
    * allowing operations on sizes that are not statically known at
      compile-time (eg variable-sized accesses).
    * more ergonomic and less brittle iteration through data (eg does not need
      manual if checking for being within bounds of data_end)

When comparing the differences in runtime for packet parsing without dynptrs
vs. with dynptrs for the more simple cases, there is no noticeable difference.
For the more complex cases where lengths are non-statically known at compile
time, there can be a significant speed-up when using dynptrs (eg a 2x speed up
for cls redirection). Patch 3 contains more details as well as examples of how
to use skb and xdp dynptrs.

[0] https://lore.kernel.org/bpf/[e-mail]/

--
Changelog:

v3 = https://lore.kernel.org/bpf/[e-mail]/
v3 -> v4
    * Forgot to commit --amend the kernel test robot error fixups

v2 = https://lore.kernel.org/bpf/[e-mail]/
v2 -> v3
    * Fix kernel test robot build test errors

v1 = https://lore.kernel.org/bpf/[e-mail]/
v1 -> v2
  * Return data slices to rd-only skb dynptrs (Martin)
  * bpf_dynptr_write allows writes to frags for skb dynptrs, but always
    invalidates associated data slices (Martin)
  * Use switch casing instead of ifs (Andrii)
  * Use 0xFD for experimental kind number in the selftest (Zvi)
  * Put selftest conversions w/ dynptrs into new files (Alexei)
  * Add new selftest "test_cls_redirect_dynptr.c" 

Joanne Koong (3):
  bpf: Add skb dynptrs
  bpf: Add xdp dynptrs
  selftests/bpf: tests for using dynptrs to parse skb and xdp buffers

 include/linux/bpf.h                           |  87 +-
 include/linux/filter.h                        |   7 +
 include/uapi/linux/bpf.h                      |  59 +-
 kernel/bpf/helpers.c                          |  93 +-
 kernel/bpf/verifier.c                         | 105 +-
 net/core/filter.c                             |  99 +-
 tools/include/uapi/linux/bpf.h                |  59 +-
 .../selftests/bpf/prog_tests/cls_redirect.c   |  25 +
 .../testing/selftests/bpf/prog_tests/dynptr.c | 132 ++-
 .../selftests/bpf/prog_tests/l4lb_all.c       |   2 +
 .../bpf/prog_tests/parse_tcp_hdr_opt.c        |  85 ++
 .../selftests/bpf/prog_tests/xdp_attach.c     |   9 +-
 .../testing/selftests/bpf/progs/dynptr_fail.c | 111 ++
 .../selftests/bpf/progs/dynptr_success.c      |  29 +
 .../bpf/progs/test_cls_redirect_dynptr.c      | 968 ++++++++++++++++++
 .../bpf/progs/test_l4lb_noinline_dynptr.c     | 468 +++++++++
 .../bpf/progs/test_parse_tcp_hdr_opt.c        | 119 +++
 .../bpf/progs/test_parse_tcp_hdr_opt_dynptr.c | 110 ++
 .../selftests/bpf/progs/test_xdp_dynptr.c     | 240 +++++
 .../selftests/bpf/test_tcp_hdr_options.h      |   1 +
 20 files changed, 2693 insertions(+), 115 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/parse_tcp_hdr_opt.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_l4lb_noinline_dynptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_parse_tcp_hdr_opt.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_parse_tcp_hdr_opt_dynptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_dynptr.c

Comments

Kumar Kartikeya Dwivedi Aug. 23, 2022, 2:31 a.m. UTC | #1

On Tue, 23 Aug 2022 at 02:06, Joanne Koong <[e-mail]> wrote:
>
> This patchset is the 2nd in the dynptr series. The 1st can be found here [0].
>
> This patchset adds skb and xdp type dynptrs, which have two main benefits for
> packet parsing:
>     * allowing operations on sizes that are not statically known at
>       compile-time (eg variable-sized accesses).
>     * more ergonomic and less brittle iteration through data (eg does not need
>       manual if checking for being within bounds of data_end)
>

Just curious: so would you be adding a dynptr interface for obtaining
data_meta slices as well in the future? Since the same manual bounds
checking is needed for data_meta vs data. How would that look in the
generic dynptr interface of data/read/write this set is trying to fit
things in?



> When comparing the differences in runtime for packet parsing without dynptrs
> vs. with dynptrs for the more simple cases, there is no noticeable difference.
> For the more complex cases where lengths are non-statically known at compile
> time, there can be a significant speed-up when using dynptrs (eg a 2x speed up
> for cls redirection). Patch 3 contains more details as well as examples of how
> to use skb and xdp dynptrs.
>
> [0] https://lore.kernel.org/bpf/[e-mail]/
>
> --

Joanne Koong Aug. 23, 2022, 6:52 p.m. UTC | #2

On Mon, Aug 22, 2022 at 7:32 PM Kumar Kartikeya Dwivedi
<[e-mail]> wrote:
>
> On Tue, 23 Aug 2022 at 02:06, Joanne Koong <[e-mail]> wrote:
> >
> > This patchset is the 2nd in the dynptr series. The 1st can be found here [0].
> >
> > This patchset adds skb and xdp type dynptrs, which have two main benefits for
> > packet parsing:
> >     * allowing operations on sizes that are not statically known at
> >       compile-time (eg variable-sized accesses).
> >     * more ergonomic and less brittle iteration through data (eg does not need
> >       manual if checking for being within bounds of data_end)
> >
>
> Just curious: so would you be adding a dynptr interface for obtaining
> data_meta slices as well in the future? Since the same manual bounds
> checking is needed for data_meta vs data. How would that look in the
> generic dynptr interface of data/read/write this set is trying to fit
> things in?

Oh cool, I didn't realize there is also a data_meta used in packet
parsing - thanks for bringing this up. I think there are 2 options for
how data_meta can be incorporated into the dynptr interface:

1) have a separate api "bpf_dynptr_from_{skb/xdp}_meta. We'll have to
have a function in the verifier that does something similar to
'may_access_direct_pkt_data' but for pkt data meta, since skb progs
can have different access restrictions for data vs. data_meta.

2) ideally, the flags arg would be used to indicate whether the
parsing should be for data_meta. To support this though, I think we'd
need to do access type checking within the helper instead of at the
verifier level. One idea is to pass in the env->ops ptr as a 4th arg
(manually patching it from the verifier) to the helper,  which can be
used to determine if data_meta access is permitted.

In both options, there'll be a new BPF_DYNPTR_{SKB/XDP}_META dynptr
type and data/read/write will be supported for it.

What are your thoughts?

>
>
>
> > When comparing the differences in runtime for packet parsing without dynptrs
> > vs. with dynptrs for the more simple cases, there is no noticeable difference.
> > For the more complex cases where lengths are non-statically known at compile
> > time, there can be a significant speed-up when using dynptrs (eg a 2x speed up
> > for cls redirection). Patch 3 contains more details as well as examples of how
> > to use skb and xdp dynptrs.
> >
> > [0] https://lore.kernel.org/bpf/[e-mail]/
> >
> > --

Andrii Nakryiko Aug. 24, 2022, 6:01 p.m. UTC | #3

On Tue, Aug 23, 2022 at 11:52 AM Joanne Koong <[e-mail]> wrote:
>
> On Mon, Aug 22, 2022 at 7:32 PM Kumar Kartikeya Dwivedi
> <[e-mail]> wrote:
> >
> > On Tue, 23 Aug 2022 at 02:06, Joanne Koong <[e-mail]> wrote:
> > >
> > > This patchset is the 2nd in the dynptr series. The 1st can be found here [0].
> > >
> > > This patchset adds skb and xdp type dynptrs, which have two main benefits for
> > > packet parsing:
> > >     * allowing operations on sizes that are not statically known at
> > >       compile-time (eg variable-sized accesses).
> > >     * more ergonomic and less brittle iteration through data (eg does not need
> > >       manual if checking for being within bounds of data_end)
> > >
> >
> > Just curious: so would you be adding a dynptr interface for obtaining
> > data_meta slices as well in the future? Since the same manual bounds
> > checking is needed for data_meta vs data. How would that look in the
> > generic dynptr interface of data/read/write this set is trying to fit
> > things in?
>
> Oh cool, I didn't realize there is also a data_meta used in packet
> parsing - thanks for bringing this up. I think there are 2 options for
> how data_meta can be incorporated into the dynptr interface:
>
> 1) have a separate api "bpf_dynptr_from_{skb/xdp}_meta. We'll have to
> have a function in the verifier that does something similar to
> 'may_access_direct_pkt_data' but for pkt data meta, since skb progs
> can have different access restrictions for data vs. data_meta.
>
> 2) ideally, the flags arg would be used to indicate whether the
> parsing should be for data_meta. To support this though, I think we'd
> need to do access type checking within the helper instead of at the
> verifier level. One idea is to pass in the env->ops ptr as a 4th arg
> (manually patching it from the verifier) to the helper,  which can be
> used to determine if data_meta access is permitted.
>
> In both options, there'll be a new BPF_DYNPTR_{SKB/XDP}_META dynptr
> type and data/read/write will be supported for it.
>
> What are your thoughts?

I think separate bpf_dynptr_from_skb_meta() and
bpf_dynptr_from_xdp_meta() is cleaner than a flag. Also having a
separate helper would make it easier to disable this helper for
program types that don't have access to ctx->data_meta, right?

>
> >
> >
> >
> > > When comparing the differences in runtime for packet parsing without dynptrs
> > > vs. with dynptrs for the more simple cases, there is no noticeable difference.
> > > For the more complex cases where lengths are non-statically known at compile
> > > time, there can be a significant speed-up when using dynptrs (eg a 2x speed up
> > > for cls redirection). Patch 3 contains more details as well as examples of how
> > > to use skb and xdp dynptrs.
> > >
> > > [0] https://lore.kernel.org/bpf/[e-mail]/
> > >
> > > --

Kumar Kartikeya Dwivedi Aug. 24, 2022, 11:18 p.m. UTC | #4

On Wed, 24 Aug 2022 at 20:02, Andrii Nakryiko <[e-mail]> wrote:
>
> On Tue, Aug 23, 2022 at 11:52 AM Joanne Koong <[e-mail]> wrote:
> >
> > On Mon, Aug 22, 2022 at 7:32 PM Kumar Kartikeya Dwivedi
> > <[e-mail]> wrote:
> > >
> > > On Tue, 23 Aug 2022 at 02:06, Joanne Koong <[e-mail]> wrote:
> > > >
> > > > This patchset is the 2nd in the dynptr series. The 1st can be found here [0].
> > > >
> > > > This patchset adds skb and xdp type dynptrs, which have two main benefits for
> > > > packet parsing:
> > > >     * allowing operations on sizes that are not statically known at
> > > >       compile-time (eg variable-sized accesses).
> > > >     * more ergonomic and less brittle iteration through data (eg does not need
> > > >       manual if checking for being within bounds of data_end)
> > > >
> > >
> > > Just curious: so would you be adding a dynptr interface for obtaining
> > > data_meta slices as well in the future? Since the same manual bounds
> > > checking is needed for data_meta vs data. How would that look in the
> > > generic dynptr interface of data/read/write this set is trying to fit
> > > things in?
> >
> > Oh cool, I didn't realize there is also a data_meta used in packet
> > parsing - thanks for bringing this up. I think there are 2 options for
> > how data_meta can be incorporated into the dynptr interface:
> >
> > 1) have a separate api "bpf_dynptr_from_{skb/xdp}_meta. We'll have to
> > have a function in the verifier that does something similar to
> > 'may_access_direct_pkt_data' but for pkt data meta, since skb progs
> > can have different access restrictions for data vs. data_meta.
> >
> > 2) ideally, the flags arg would be used to indicate whether the
> > parsing should be for data_meta. To support this though, I think we'd
> > need to do access type checking within the helper instead of at the
> > verifier level. One idea is to pass in the env->ops ptr as a 4th arg
> > (manually patching it from the verifier) to the helper,  which can be
> > used to determine if data_meta access is permitted.
> >
> > In both options, there'll be a new BPF_DYNPTR_{SKB/XDP}_META dynptr
> > type and data/read/write will be supported for it.
> >
> > What are your thoughts?
>
> I think separate bpf_dynptr_from_skb_meta() and
> bpf_dynptr_from_xdp_meta() is cleaner than a flag. Also having a
> separate helper would make it easier to disable this helper for
> program types that don't have access to ctx->data_meta, right?
>

Agreed, and with flags then you also need to force them to be constant
(to be able to distinguish the return type from the flag's value),
which might be too restrictive.