Re: [PATCH net] net: sched: act_mirred: drop skb's dst_entry in ingress redirection

[Zhiyuan Hou]

On 2019/10/19 5:25 äå, Eyal Birger wrote:
> Hi,
>
> On Fri, 18 Oct 2019 00:33:53 +0800
> Zhiyuan Hou <zhiyuan2048@xxxxxxxxxxxxxxxxx> wrote:
>
> > On 2019/10/16 8:13 äå, Eyal Birger wrote:
> > > Hi,
> > >
> > > On Wed, 16 Oct 2019 01:22:01 +0800
> > > Zhiyuan Hou <zhiyuan2048@xxxxxxxxxxxxxxxxx> wrote:
> > >   
> > > > On 2019/10/15 1:57 äå, Cong Wang wrote:
> > > > > On Sat, Oct 12, 2019 at 12:16 AM Zhiyuan Hou
> > > > > <zhiyuan2048@xxxxxxxxxxxxxxxxx> 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.