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

[Zhiyuan Hou]

On 2019/10/14 8:46 äå, Eric Dumazet wrote:
> On 10/14/19 12:07 AM, Zhiyuan Hou wrote:
> > On 2019/10/12 6:59 äå, Eric Dumazet wrote:
> > > On 10/12/19 12:16 AM, Zhiyuan Hou wrote:
> > > > In act_mirred's ingress redirection, if the skb's dst_entry is valid
> > > > when call function netif_receive_skb, the fllowing l3 stack process
> > > > (ip_rcv_finish_core) will check dst_entry and skip the routing
> > > > decision. Using the old dst_entry is unexpected and may discard the
> > > > skb in some case. For example dst->dst_input points to dst_discard.
> > > >
> > > > This patch drops the skb's dst_entry before calling netif_receive_skb
> > > > so that the skb can be made routing decision like a normal ingress
> > > > skb.
> > > >
> > > > Signed-off-by: Zhiyuan Hou <zhiyuan2048@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  Â net/sched/act_mirred.c | 5 ++++-
> > > >  Â 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > 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);
> > > > +ÂÂÂ }
> > > >  Â ÂÂÂÂÂ if (err) {
> > > >  Â out:
> > > Why is dst_discard used ?
> > When send a skb from local to external, the dst->dst_input will be
> > assigned dst_discard after routing decision. So if we redirect these
> > skbs to ingress stack, it will be dropped.
> >
> > For ipvlan l2 mode or macvlan, clsact egress filters on master deivce
> > may also meet these skbs even if they came from slave device. Ingress
> > redirection on these skbs may drop them on l3 stack.
> Can you please add a test, so that we can see what you are trying to do exactly ?
Sure. Suppose a linux box has two interfaces (eth0 and eth1). We
create a vrf (vrf0) and put eth1 in it, as following commands:

 # ip link add vrf0 type vrf table 10
 # ip link set dev vrf0 up
 # ip link set eth1 master vrf0

Then let's intercept some egress flows through eth0 and redirect them
to vrf0 using act_mirred.

 # ip route add table 10 ... # add routes to vrf0
 # tc qdisc add dev eth0 clsact
 # tc filter add dev eth0 egress proto ip u32 \
ÂÂÂÂÂÂ match ip dst 10.0.0.0/24 \
ÂÂÂÂÂÂ action mirred ingress redirect dev eth1

We expect that the matching skb will be received or forwarded via
vrf0's route table. But the fact is, the skb is dropped as commit log
notes.
> > > This could actually drop packets, for loopback.
> > >
> > > A Fixes: tag would tremendously help, I wonder if you are not working around
> > > the other issue Wei was tracking yesterday ( https://www.spinics.net/lists/netdev/msg604397.html )
> > No, this is a different issue ^_^.
> Please add a Fixes: tag then.
Yes, I will add following tag in v2.
Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
> Thanks.