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]
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

Powered by blists - more mailing lists