netdev - Re: [PATCH net] net: sched: act_mirred: drop skb's dst

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: <5e9f0ae7-d31e-a428-9780-b6b7130f73f8@linux.alibaba.com>
Date:   Mon, 21 Oct 2019 21:06:42 +0800
From:   Zhiyuan Hou <zhiyuan2048@...ux.alibaba.com>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, shmulik.ladkani@...il.com
Subject: Re: [PATCH net] net: sched: act_mirred: drop skb's dst_entry in
 ingress redirection


On 2019/10/19 5:25 上午, Eyal Birger wrote:
> Hi,
>
> On Fri, 18 Oct 2019 00:33:53 +0800
> Zhiyuan Hou <zhiyuan2048@...ux.alibaba.com> wrote:
>
>> On 2019/10/16 8:13 下午, Eyal Birger wrote:
>>> Hi,
>>>
>>> On Wed, 16 Oct 2019 01:22:01 +0800
>>> Zhiyuan Hou <zhiyuan2048@...ux.alibaba.com> wrote:
>>>   
>>>> On 2019/10/15 1:57 上午, Cong Wang wrote:
>>>>> On Sat, Oct 12, 2019 at 12:16 AM Zhiyuan Hou
>>>>> <zhiyuan2048@...ux.alibaba.com> wrote:
>>>>>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>>>>>> index 9ce073a05414..6108a64c0cd5 100644
>>>>>> --- a/net/sched/act_mirred.c
>>>>>> +++ b/net/sched/act_mirred.c
>>>>>> @@ -18,6 +18,7 @@
>>>>>>     #include <linux/gfp.h>
>>>>>>     #include <linux/if_arp.h>
>>>>>>     #include <net/net_namespace.h>
>>>>>> +#include <net/dst.h>
>>>>>>     #include <net/netlink.h>
>>>>>>     #include <net/pkt_sched.h>
>>>>>>     #include <net/pkt_cls.h>
>>>>>> @@ -298,8 +299,10 @@ static int tcf_mirred_act(struct sk_buff
>>>>>> *skb, const struct tc_action *a,
>>>>>>
>>>>>>            if (!want_ingress)
>>>>>>                    err = dev_queue_xmit(skb2);
>>>>>> -       else
>>>>>> +       else {
>>>>>> +               skb_dst_drop(skb2);
>>>>>>                    err = netif_receive_skb(skb2);
>>>>>> +       }
>>>>> Good catch!
>>> Indeed! Thanks for fixing this!
>>>   
>>>>> I don't want to be picky, but it seems this is only needed
>>>>> when redirecting from egress to ingress, right? That is,
>>>>> ingress to ingress, or ingress to egress is okay? If not,
>>>>> please fix all the cases while you are on it?
>>>> Sure. But I think this patch is also needed when redirecting from
>>>> ingress to ingress. Because we cannot assure that a skb has null
>>>> dst in ingress redirection path. For example, if redirecting a skb
>>>> from loopback's ingress to other device's ingress, the skb will
>>>> take a dst.
>>>>
>>>> As commit logs point out, skb with valid dst cannot be made routing
>>>> decision in following process. original dst may cause skb loss or
>>>> other unexpected behavior.
>>> On the other hand, removing the dst on ingress-to-ingress
>>> redirection may remove LWT information on incoming packets, which
>>> may be undesired.
>> Sorry, I do not understand why lwt information is needed on
>> ingress-to-ingress redirection. lwt is used on output path, isn't it?
>> Can you please give more information?
> On rx path tunnelled packets parameters received on a collect_md tunnel device
> are kept in a metadata dst. See ip_tunnel_rcv() 'tun_dst' parameter.
>
> The rx metadata dst can be matched by a number of mechanisms like routing
> rules, eBPF, OVS, and netfilter.
Yes, you are right. Thanks for your explanations.

The metadata dst should not be removed in redirection path and also
does not affect L3's routing decision.

Maybe we can add a following check to solve it before removing a dst,
what do you think?

   if (skb_valid_dst(skb2))
       skb_dst_drop(sbk2);
>
> Eyal.