[xdp-hints] Re: [PATCH RFC bpf-next 5/9] xdp: controlling XDP-hints from BPF-prog via helper

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

Jesper Dangaard Brouer <brouer@redhat.com> writes:

> XDP BPF-prog's need a way to interact with the XDP-hints. This patch
> introduces a BPF-helper function, that allow XDP BPF-prog's to interact
> with the XDP-hints.
>
> BPF-prog can query if any XDP-hints have been setup and if this is
> compatible with the xdp_hints_common struct. If XDP-hints are available
> the BPF "origin" is returned (see enum xdp_hints_btf_origin) as BTF can
> come from different sources or origins e.g. vmlinux, module or local.

I'm not sure I quite understand what this origin is supposed to be good
for? What is a BPF (or AF_XDP) program supposed to do with the
information "this XDP hints struct came from a module?" without knowing
which module that was? Ultimately, the origin is useful for a consumer
to check that the metadata is in the format that it's expecting it to be
in (so it can just load the data from the appropriate offsets). But to
answer this, we really need a unique identifier; so I think the approach
in Alexander's series of encoding the ID of the BTF structure itself
into the next 32 bits is better? That way we'll have a unique "pointer"
to the actual struct that's in the metadata area and can act on this.

> RFC/TODO: Improve patch: Can verifier validate provided BTF on "update"
> and detect if compatible with common struct???

If we have the unique ID as mentioned above, I think the kernel probably
could resolve this automatically: whenever a module is loaded, the
kernel could walk the BTF information from that module an simply inspect
all the metadata structs and see if they contain the embedded
xdp_hints_common struct. The IDs of any metadata structs that do contain
the common struct can then be kept in a central lookup table and the
consumption code can then simply compare the BTF ID to this table when
building an SKB?

As for the validation on the BPF side:n

> +	if (flags & HINTS_BTF_UPDATE) {
> +		is_compat_common = !!(flags & HINTS_BTF_COMPAT_COMMON);
> +	/* TODO: Can kernel validate if hints are BTF compat with common? */
> +	/* TODO: Could BPF prog provide BTF as ARG_PTR_TO_BTF_ID to prove compat_common ? */

If we use the "global ID + lookup table" approach above, we don't really
need to validate anything here: if the program says it's writing
metadata with a format given by a specific ID, that implies
compatibility (or not) as given by the ID. We could sanity-check the
metadata area size, but the consumption code has to do that anyway, so
I'm not sure it's worth the runtime overhead to have an additional check
here?

As for safety of the metadata content itself, I don't really think we
can do anything to guarantee this: in any case the BPF program can pass
a valid BTF ID and still write garbage values into the actual fields, so
the consumption code has to do enough validation that this won't crash
the kernel anyway. But this is no different from the packet data itself:
XDP is basically in a position to be a MITM attacker of the network
stack itself, which is why loading XDP programs is a privileged
operation...

-Toke