Re: [PATCH bpf v2 1/2] bpf: support SKF_NET_OFF and SKF_LL_OFF on skb frags

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Daniel Borkmann wrote:
> On 4/4/25 7:56 PM, Maciej Żenczykowski wrote:
> > On Fri, Apr 4, 2025 at 9:34 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> >> On Fri, Apr 4, 2025 at 12:11 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>
> >>> Hi Willem,
> >>>
> >>> On 4/4/25 4:23 PM, Willem de Bruijn wrote:
> >>> [...]
> >>>> v1->v2
> >>>>     - introduce bfp_skb_load_helper_convert_offset to avoid open coding
> >>>> ---
> >>>>    include/linux/filter.h |  3 --
> >>>>    kernel/bpf/core.c      | 21 -----------
> >>>>    net/core/filter.c      | 80 +++++++++++++++++++++++-------------------
> >>>>    3 files changed, 44 insertions(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >>>> index f5cf4d35d83e..708ac7e0cd36 100644
> >>>> --- a/include/linux/filter.h
> >>>> +++ b/include/linux/filter.h
> >>>> @@ -1496,9 +1496,6 @@ static inline u16 bpf_anc_helper(const struct sock_filter *ftest)
> >>>>        }
> >>>>    }
> >>>>
> >>>> -void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb,
> >>>> -                                        int k, unsigned int size);
> >>>> -
> >>>>    static inline int bpf_tell_extensions(void)
> >>>>    {
> >>>>        return SKF_AD_MAX;
> >>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> >>>> index ba6b6118cf50..0e836b5ac9a0 100644
> >>>> --- a/kernel/bpf/core.c
> >>>> +++ b/kernel/bpf/core.c
> >>>> @@ -68,27 +68,6 @@
> >>>>    struct bpf_mem_alloc bpf_global_ma;
> >>>>    bool bpf_global_ma_set;
> >>>>
> >>>> -/* No hurry in this branch
> >>>> - *
> >>>> - * Exported for the bpf jit load helper.
> >>>> - */
> >>>> -void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
> >>>> -{
> >>>> -     u8 *ptr = NULL;
> >>>> -
> >>>> -     if (k >= SKF_NET_OFF) {
> >>>> -             ptr = skb_network_header(skb) + k - SKF_NET_OFF;
> >>>> -     } else if (k >= SKF_LL_OFF) {
> >>>> -             if (unlikely(!skb_mac_header_was_set(skb)))
> >>>> -                     return NULL;
> >>>> -             ptr = skb_mac_header(skb) + k - SKF_LL_OFF;
> >>>> -     }
> >>>> -     if (ptr >= skb->head && ptr + size <= skb_tail_pointer(skb))
> >>>> -             return ptr;
> >>>> -
> >>>> -     return NULL;
> >>>> -}
> >>>
> >>> Wouldn't this break sparc 32bit JIT which still calls into this?
> >>>
> >>> arch/sparc/net/bpf_jit_asm_32.S :
> >>>
> >>> #define bpf_negative_common(LEN)                        \
> >>>           save    %sp, -SAVE_SZ, %sp;                     \
> >>>           mov     %i0, %o0;                               \
> >>>           mov     r_OFF, %o1;                             \
> >>>           SIGN_EXTEND(%o1);                               \
> >>>           call    bpf_internal_load_pointer_neg_helper;   \
> >>>            mov    (LEN), %o2;                             \
> >>>           mov     %o0, r_TMP;                             \
> >>>           cmp     %o0, 0;                                 \
> >>>           BE_PTR(bpf_error);                              \
> >>>            restore;
> >>
> >> Argh, good catch. Thanks Daniel.
> >>
> >> I'll drop the removal of bpf_internal_load_pointer_neg_helper from the patch.
> > 
> > add a 'deprecated only used by sparc32 comment'
> > 
> > hopefully someone that knows sparc32 assembly can fix it
> 
> Alternatively, the bpf_internal_load_pointer_neg_helper() could be moved entirely
> over into arch/sparc/net/ so that others won't be tempted to reuse.

I'd prefer to keep it as is.

I took a stab, but my Debian has no sparc32 gcc cross compiler anymore,
and I was unable to cross compile with clang either.