[v4,bpf-next,0/5] bpf: Add support for XDP programs in DEVMAP entries

Implementation of Daniel's proposal for allowing DEVMAP entries to be
a device index, program fd pair.

Programs are run after XDP_REDIRECT and have access to both Rx device
and Tx device.

v4
- moved struct bpf_devmap_val from uapi to devmap.c, named the union
  and dropped the prefix from the elements - Jesper
- fixed 2 bugs in selftests

v3
- renamed struct to bpf_devmap_val
- used offsetofend to check for expected map size, modification of
  Toke's comment
- check for explicit value sizes
- adjusted switch statement in dev_map_run_prog per Andrii's comment
- changed SEC shortcut to xdp_devmap
- changed selftests to use skeleton and new map declaration

v2
- moved dev_map_ext_val definition to uapi to formalize the API for devmap
  extensions; add bpf_ prefix to the prog_fd and prog_id entries
- changed devmap code to handle struct in a way that it can support future
  extensions
- fixed subject in libbpf patch

v1
- fixed prog put on invalid program - Toke
- changed write value from id to fd per Toke's comments about capabilities
- add test cases

David Ahern (5):
  devmap: Formalize map value as a named struct
  bpf: Add support to attach bpf program to a devmap entry
  xdp: Add xdp_txq_info to xdp_buff
  libbpf: Add SEC name for xdp programs attached to device map
  selftest: Add tests for XDP programs in devmap entries

 include/linux/bpf.h                           |   5 +
 include/net/xdp.h                             |   5 +
 include/uapi/linux/bpf.h                      |   3 +
 kernel/bpf/devmap.c                           | 130 +++++++++++++++---
 net/core/dev.c                                |  18 +++
 net/core/filter.c                             |  17 +++
 tools/include/uapi/linux/bpf.h                |   3 +
 tools/lib/bpf/libbpf.c                        |   2 +
 .../bpf/prog_tests/xdp_devmap_attach.c        |  97 +++++++++++++
 .../bpf/progs/test_xdp_devmap_helpers.c       |  22 +++
 .../bpf/progs/test_xdp_with_devmap_helpers.c  |  44 ++++++
 11 files changed, 328 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap_helpers.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c

On Fri, May 29, 2020 at 3:07 PM David Ahern <dsahern@kernel.org> wrote:
>
> Implementation of Daniel's proposal for allowing DEVMAP entries to be
> a device index, program fd pair.
>
> Programs are run after XDP_REDIRECT and have access to both Rx device
> and Tx device.
>
> v4
> - moved struct bpf_devmap_val from uapi to devmap.c, named the union
>   and dropped the prefix from the elements - Jesper
> - fixed 2 bugs in selftests

Applied.

In patch 5 I had to fix:
/data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:
In function ‘test_neg_xdp_devmap_helpers’:
/data/users/ast/net-next/tools/testing/selftests/bpf/test_progs.h:106:3:
warning: ‘duration’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
  106 |   fprintf(stdout, "%s:PASS:%s %d nsec\n",   \
      |   ^~~~~~~
/data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:79:8:
note: ‘duration’ was declared here
   79 |  __u32 duration;

and that selftest is imo too primitive.
It's only loading progs and not executing them.
Could you please add prog_test_run to it?

On 6/1/20 3:12 PM, Alexei Starovoitov wrote:
> In patch 5 I had to fix:
> /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:
> In function ‘test_neg_xdp_devmap_helpers’:
> /data/users/ast/net-next/tools/testing/selftests/bpf/test_progs.h:106:3:
> warning: ‘duration’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>   106 |   fprintf(stdout, "%s:PASS:%s %d nsec\n",   \
>       |   ^~~~~~~
> /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:79:8:
> note: ‘duration’ was declared here
>    79 |  __u32 duration;

What compiler version? it compiles cleanly with ubuntu 20.04 and gcc
9.3. The other prog_tests are inconsistent with initializing it.

> 
> and that selftest is imo too primitive.

I focused the selftests on API changes introduced by this set - new
attach type, valid accesses to egress_ifindex and not allowing devmap
programs with xdp generic.

> It's only loading progs and not executing them.
> Could you please add prog_test_run to it?
> 

I will look into it.

On Mon, Jun 01, 2020 at 04:28:02PM -0600, David Ahern wrote:
> On 6/1/20 3:12 PM, Alexei Starovoitov wrote:
> > In patch 5 I had to fix:
> > /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:
> > In function ‘test_neg_xdp_devmap_helpers’:
> > /data/users/ast/net-next/tools/testing/selftests/bpf/test_progs.h:106:3:
> > warning: ‘duration’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> >   106 |   fprintf(stdout, "%s:PASS:%s %d nsec\n",   \
> >       |   ^~~~~~~
> > /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:79:8:
> > note: ‘duration’ was declared here
> >    79 |  __u32 duration;
> 
> What compiler version? it compiles cleanly with ubuntu 20.04 and gcc
> 9.3. The other prog_tests are inconsistent with initializing it.

official rhel's devtoolset-9
gcc version 9.1.1 20190605 (Red Hat 9.1.1-2) (GCC)

> > 
> > and that selftest is imo too primitive.
> 
> I focused the selftests on API changes introduced by this set - new
> attach type, valid accesses to egress_ifindex and not allowing devmap
> programs with xdp generic.
> 
> > It's only loading progs and not executing them.
> > Could you please add prog_test_run to it?
> > 
> 
> I will look into it.

Thanks!

On Mon, Jun 1, 2020 at 3:28 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 6/1/20 3:12 PM, Alexei Starovoitov wrote:
> > In patch 5 I had to fix:
> > /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:
> > In function ‘test_neg_xdp_devmap_helpers’:
> > /data/users/ast/net-next/tools/testing/selftests/bpf/test_progs.h:106:3:
> > warning: ‘duration’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> >   106 |   fprintf(stdout, "%s:PASS:%s %d nsec\n",   \
> >       |   ^~~~~~~
> > /data/users/ast/net-next/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c:79:8:
> > note: ‘duration’ was declared here
> >    79 |  __u32 duration;
>
> What compiler version? it compiles cleanly with ubuntu 20.04 and gcc
> 9.3. The other prog_tests are inconsistent with initializing it.

Do you have specific examples of inconsistencies? Seems like duration is:
1. either static variable, and thus zero-initialized;
2. is initialized explicitly at declaration;
3. is filled out with bpf_prog_test_run().

>
> >
> > and that selftest is imo too primitive.
>
> I focused the selftests on API changes introduced by this set - new
> attach type, valid accesses to egress_ifindex and not allowing devmap
> programs with xdp generic.
>
> > It's only loading progs and not executing them.
> > Could you please add prog_test_run to it?
> >
>
> I will look into it.

On 6/1/20 4:52 PM, Andrii Nakryiko wrote:
> Do you have specific examples of inconsistencies? Seems like duration is:

nope, just a quick grep trying to understand why it compiled cleanly for
me and looking at similar tests.

> 1. either static variable, and thus zero-initialized;
> 2. is initialized explicitly at declaration;
> 3. is filled out with bpf_prog_test_run().
> 

apparently so.

On 6/1/20 4:28 PM, David Ahern wrote:
>>
>> and that selftest is imo too primitive.
> 
> I focused the selftests on API changes introduced by this set - new
> attach type, valid accesses to egress_ifindex and not allowing devmap
> programs with xdp generic.
> 
>> It's only loading progs and not executing them.
>> Could you please add prog_test_run to it?
>>
> 
> I will look into it.
> 

The test_run infrastructure does not handle XDP_REDIRECT which is needed
to step into the devmap code and then run programs tied to devmap entries.