On Tue, 18 Jul 2023 13:34:06 -0700 Alexei Starovoitov wrote:
Direct packet access via skb->data is there for those who want high speed 🤷️
skb->data/data_end approach unfortunately doesn't work that well. Too much verifier fighting. That's why dynptr was introduced.
I wish Daniel told us more about the use case.
My worry is that people will think that whether the buffer is needed or not depends on _their program_, rather than on the underlying platform. So if it works in testing without the buffer - the buffer must not be required for their use case.
Are you concerned about bpf progs breaking this way?
Both, BPF progs breaking and netdev code doing things which don't make sense. But I won't argue too hard about the former, i.e. the BPF API.
I thought you're worried about the driver misusing skb_header_pointer() with buffer==NULL.
We can remove !buffer check as in the attached patch, but I don't quite see how it would improve driver quality.
The drivers may not be pretty but they aren't buggy AFAICT.
[0001-bpf-net-Introduce-skb_pointer_if_linear.patch application/octet-stream (2873 bytes)]
Or we can simply pretend we don't have the skb:
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 91ed66952580..217447f01d56 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -4023,7 +4023,7 @@ __skb_header_pointer(const struct sk_buff *skb, int offset, int len, if (likely(hlen - offset >= len)) return (void *)data + offset;
- if (!skb || !buffer || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0)) + if (!skb || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0)) return NULL;
return buffer; diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 9e80efa59a5d..8bc4622cc1df 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2239,7 +2239,13 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset case BPF_DYNPTR_TYPE_RINGBUF: return ptr->data + ptr->offset + offset; case BPF_DYNPTR_TYPE_SKB: - return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt); + { + const struct sk_buff *skb = ptr->data; + + return __skb_header_pointer(NULL, ptr->offset + offset, len, + skb->data, skb_headlen(skb), + buffer__opt); + } case BPF_DYNPTR_TYPE_XDP: { void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);