netdev - Re: [net v6 1/2] net: core: set skb useful vars in __bpf_tx

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] [day] [month] [year] [list]

Message-ID: <fe259f3b-8808-5d4c-46b3-7031ee61a63e@iogearbox.net>
Date:   Wed, 30 Mar 2022 14:00:09 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Antoine Tenart <atenart@...nel.org>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Wei Wang <weiwan@...gle.com>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [net v6 1/2] net: core: set skb useful vars in __bpf_tx_skb

On 3/25/22 1:56 AM, Tonghao Zhang wrote:
> On Thu, Mar 24, 2022 at 11:16 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
>>
>> On Thu, Mar 24, 2022 at 6:57 AM <xiangxia.m.yue@...il.com> wrote:
>>>
>>> From: Tonghao Zhang <xiangxia.m.yue@...il.com>
>>>
>>> We may use bpf_redirect to redirect the packets to other
>>> netdevice (e.g. ifb) in ingress or egress path.
>>>
>>> The target netdevice may check the *skb_iif, *redirected
>>> and *from_ingress. For example, if skb_iif or redirected
>>> is 0, ifb will drop the packets.
>>>
>>> Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
>>> Cc: "David S. Miller" <davem@...emloft.net>
>>> Cc: Jakub Kicinski <kuba@...nel.org>
>>> Cc: Alexei Starovoitov <ast@...nel.org>
>>> Cc: Daniel Borkmann <daniel@...earbox.net>
>>> Cc: Andrii Nakryiko <andrii@...nel.org>
>>> Cc: Martin KaFai Lau <kafai@...com>
>>> Cc: Song Liu <songliubraving@...com>
>>> Cc: Yonghong Song <yhs@...com>
>>> Cc: John Fastabend <john.fastabend@...il.com>
>>> Cc: KP Singh <kpsingh@...nel.org>
>>> Cc: Eric Dumazet <edumazet@...gle.com>
>>> Cc: Antoine Tenart <atenart@...nel.org>
>>> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>
>>> Cc: Wei Wang <weiwan@...gle.com>
>>> Cc: Arnd Bergmann <arnd@...db.de>
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
>>> ---
>>>   net/core/filter.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index a7044e98765e..c1f45d2e6b0a 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2107,7 +2107,15 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>>>          }
>>>
>>>          skb->dev = dev;
>>> +       /* The target netdevice (e.g. ifb) may use the:
>>> +        * - redirected
>>> +        * - from_ingress
>>> +        */
>>> +#ifdef CONFIG_NET_CLS_ACT
>>> +       skb_set_redirected(skb, skb->tc_at_ingress);
>>> +#else
>>>          skb_clear_tstamp(skb);
>>> +#endif
>>
>> I thought Daniel Nacked it a couple times already.
>> Please stop this spam.
> Hi
> Daniel rejected the 2/3 patch,
> https://patchwork.kernel.org/project/netdevbpf/patch/20211208145459.9590-3-xiangxia.m.yue@gmail.com/
> The reasons are as follows.
> * 2/3 patch adds a check in fastpath.
> * on egress, redirect skb to ifb is not useful.
> but this patch fixes  redirect skb to ifb on ingress. I think it is
> useful for us.
> 
> Daniel, can you review this patch ?

Still nack, above makes tc forwarding path for BPF less predictable and could break
existing setups, e.g. if someone is relying on generic XDP. I'm certain you can
resolve this without ifb hack, ifb usage is really discouraged in general.

Thanks,
Daniel