netdev - Re: [PATCH bpf v2 1/2] bpf: support SKF_NET_OFF and SKF_LL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98b2c012-dcbe-4abf-8b22-2ab37604ccc8@iogearbox.net>
Date: Mon, 7 Apr 2025 11:00:38 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Maciej Żenczykowski <maze@...gle.com>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
 john.fastabend@...il.com, Willem de Bruijn <willemb@...gle.com>,
 Matt Moeller <moeller.matt@...il.com>
Subject: Re: [PATCH bpf v2 1/2] bpf: support SKF_NET_OFF and SKF_LL_OFF on skb
 frags

On 4/4/25 7:56 PM, Maciej Żenczykowski wrote:
> On Fri, Apr 4, 2025 at 9:34 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
>> On Fri, Apr 4, 2025 at 12:11 PM Daniel Borkmann <daniel@...earbox.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.

Cheers,
Daniel