Re: [PATCH RFC v4 bpf-next 00/11] Add support for XDP in egress path

["Toke Høiland-Jørgensen" <toke@redhat.com>]

David Ahern <dsahern@kernel.org> writes:

> From: David Ahern <dsahern@gmail.com>
>
> This series adds support for XDP in the egress path by introducing
> a new XDP attachment type, BPF_XDP_EGRESS, and adding an if_link API
> for attaching the program to a netdevice and reporting the program.
> The intent is to emulate the current RX path for XDP as much as
> possible to maintain consistency and symmetry in the 2 paths with
> their APIs and when the programs are run: at first touch in the Rx
> path and last touch in the Tx path.
>
> The intent is to be able to run bpf programs on all packets regardless
> of how they got to the xmit function of the netdevice - as an skb or a
> redirected xdp frame. This is a missing primitive for XDP allowing
> solutions to build small, targeted programs properly distributed in the
> networking path allowing for example an egress firewall / ACL / traffic
> verification or packet manipulation and encapping an entire ethernet
> frame whether it is locally generated traffic, forwarded via the slow
> path (ie., full stack processing) or xdp redirected frames.

I'm totally on board with these goals!

As for this:

> Attempting to tag the EGRESS path as yet another mode is inconsistent
> on a number of levels - from the current usage of XDP_FLAGS to options
> passed to the verifier for restricting xdp_md accesses. Using the API
> as listed above maintains consistency with all existing code.

You *are* effectively tagging the EGRESS path as another mode: You are
restricting which fields of the context object the program can access,
and you're restricting where the program can be attached. I am pretty
sure we will end up accumulating more differences, either in more
metadata that is only available in one mode (we've already discussed
exposing TX qlen on egress programs), or even helpers that only make
sense in one mode.

So it doesn't make sense to discuss whether egress programs are a
distinct type from ingress programs: They clearly are. What we are
discussing is how to encode this type difference. You are proposing to
encode it using expected_attach_type as a subtype identifier instead of
using a new type number. There is already precedence for this with the
tracing programs, and I do think it makes sense - ingress and egress XDP
programs are clearly related, just as (e.g.) fentry/fexit/freplace
programs are.

However, my issue with this encoding is that it is write-only: You can't
inspect a BPF program already loaded into the kernel and tell which type
it is. So my proposal would be to make it explicit: Expose the
expected_attach_type as a new field in bpf_prog_info so userspace can
query it, and clearly document it as, essentially, a program subtype
that can significantly affect how a program is treated by the kernel.

-Toke