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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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?
Eyal.



[Index of Archives]

  Powered by Linux

[Older Kernel Discussion]     [Yosemite National Park Forum]     [Gimp]     [Stuff]     [Index of Other Archives]