Re: [PATCH v3] net: netfilter: Fix rpfilter dropping vrf packets by mistake

On Wed, Jun 19, 2019 at 09:49:04AM +0000, linmiaohe wrote:

On 2019/6/18 23:58, Pablo Neira Ayuso wrote:

quoted

On Thu, Apr 25, 2019 at 09:43:53PM +0800, linmiaohe wrote:

quoted

From: Miaohe Lin <linmiaohe@huawei.com>

When firewalld is enabled with ipv4/ipv6 rpfilter, vrf
ipv4/ipv6 packets will be dropped because in device is vrf but out 
device is an enslaved device. So failed with the check of the 
rpfilter.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---

--- a/net/ipv4/netfilter/ipt_rpfilter.c
+++ b/net/ipv4/netfilter/ipt_rpfilter.c

@@ -81,6 +81,7 @@ static bool rpfilter_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	flow.flowi4_mark = info->flags & XT_RPFILTER_VALID_MARK ? skb->mark : 0;
 	flow.flowi4_tos = RT_TOS(iph->tos);
 	flow.flowi4_scope = RT_SCOPE_UNIVERSE;
+	flow.flowi4_oif = l3mdev_master_ifindex_rcu(xt_in(par));

 	return rpfilter_lookup_reverse(xt_net(par), &flow, xt_in(par),
--- a/net/ipv6/netfilter/ip6t_rpfilter.c
+++ b/net/ipv6/netfilter/ip6t_rpfilter.c

@@ -58,7 +58,9 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
 	if (rpfilter_addr_linklocal(&iph->saddr)) {
 		lookup_flags |= RT6_LOOKUP_F_IFACE;
 		fl6.flowi6_oif = dev->ifindex;
-	} else if ((flags & XT_RPFILTER_LOOSE) == 0)
+	} else if (((flags & XT_RPFILTER_LOOSE) == 0) ||
+		   (netif_is_l3_master(dev)) ||
+		   (netif_is_l3_slave(dev)))
 		fl6.flowi6_oif = dev->ifindex;

 	rt = (void *)ip6_route_lookup(net, &fl6, skb, lookup_flags); @@
-73,6 +75,12 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
 		goto out;
 	}

+	if (netif_is_l3_master(dev)) {
+		dev = dev_get_by_index_rcu(dev_net(dev), IP6CB(skb)->iif);
+		if (!dev)
+			goto out;
+	}

So, for the l3 device cases this makes:

#1 ip6_route_lookup() to fetch the route, using the device in xt_in()
   (the _LOOSE flag is ignored for the l3 device case).

#2 If this is a l3dev master, then you make a global lookup for the
   device using IP6CB(skb)->iif.

#3 You check if route matches with the device, using the new device
   from the lookup:

   if (rt->rt6i_idev->dev == dev ...

If there is no other way to fix this, OK, that's fair enough.

Still this fix looks a bit tricky to me.

And this assymmetric between the IPv4 and IPv6 codebase looks rare.

Probably someone can explain me this in more detail? I'd appreciate.

Thanks!

Thanks for explaining.

Something must be wrong in all these helper function logic because this new code logic is hard to follow for the IPv6 chunk...

Can you explore a more readable fix?

So I'm not inclined to quickly take this patch.

Thanks.

Thanks, I will try a more readable fix.