Re: [PATCH bpf-next v1 1/3] bpf/xdp: Add bpf_load_hdr_opt support for xdp

On Fri, Oct 1, 2021 at 3:04 PM Joanne Koong [off-list ref] wrote:

This patch enables XDP programs to use the bpf_load_hdr_opt helper
function to load header options.

The upper 16 bits of "flags" is used to denote the offset to the tcp
header. No other flags are, at this time, used by XDP programs.
In the future, more flags can be included to support other types of
header options.

Much of the logic for loading header options can be shared between
sockops and xdp programs. In net/core/filter.c, this common shared
logic is refactored into a separate function both sockops and xdp
use.

Signed-off-by: Joanne Koong <redacted>

Looks good over all.

Acked-by: Song Liu <redacted>

Just a nitpick below.

---
 include/uapi/linux/bpf.h       | 26 ++++++----
 net/core/filter.c              | 88 ++++++++++++++++++++++++++--------
 tools/include/uapi/linux/bpf.h | 26 ++++++----
 3 files changed, 103 insertions(+), 37 deletions(-)

[...]

+
+BPF_CALL_4(bpf_xdp_load_hdr_opt, struct xdp_buff *, xdp,
+          void *, search_res, u32, len, u64, flags)
+{
+       const void *op, *opend;
+       struct tcphdr *th;
+
+       /* The upper 16 bits of flags contain the offset to the tcp header.
+        * No other bits should be set.
+        */
+       if (flags & 0xffffffffffff)

Maybe use (1ULL << BPF_LOAD_HDR_OPT_TCP_OFFSET_SHIFT) - 1

+               return -EINVAL;
+
+       th = xdp->data + (flags >> BPF_LOAD_HDR_OPT_TCP_OFFSET_SHIFT);
+       op = (void *)th + sizeof(struct tcphdr);
+       if (unlikely(op > xdp->data_end))
+               return -EINVAL;

[...]