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

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

[linmiaohe]

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>
---
 net/ipv4/netfilter/ipt_rpfilter.c  |  1 +
 net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
index 0b10d8812828..6e07cd0ecbec 100644
--- 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), info->flags) ^ invert;
 }
diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
index c3c6b09acdc4..a28c81322148 100644
--- 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;
+	}
+
 	if (rt->rt6i_idev->dev == dev || (flags & XT_RPFILTER_LOOSE))
 		ret = true;
  out:
-- 
2.19.1

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

[David Ahern]

On 4/25/19 7:43 AM, linmiaohe wrote:
> 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>
> ---
>  net/ipv4/netfilter/ipt_rpfilter.c  |  1 +
>  net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 


Reviewed-by: David Ahern <dsahern@gmail.com>

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

[Pablo Neira Ayuso]

On Thu, Apr 25, 2019 at 09:43:53PM +0800, linmiaohe wrote:
> 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>
> ---
>  net/ipv4/netfilter/ipt_rpfilter.c  |  1 +
>  net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
> index 0b10d8812828..6e07cd0ecbec 100644
> --- 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), info->flags) ^ invert;
>  }
> diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> index c3c6b09acdc4..a28c81322148 100644
> --- 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;
> +	}

Suggestion: Could you just call l3mdev_master_ifindex_rcu() when
invoking rpfilter_lookup_reverse6() ?

diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
index c3c6b09acdc4..ce64ff5d6fb6 100644
--- a/net/ipv6/netfilter/ip6t_rpfilter.c
+++ b/net/ipv6/netfilter/ip6t_rpfilter.c
@@ -101,7 +101,8 @@ static bool rpfilter_mt(const struct sk_buff *skb,
struct xt_action_param *par)
        if (unlikely(saddrtype == IPV6_ADDR_ANY))
                return true ^ invert; /* not routable: forward path will drop it */
 
-       return rpfilter_lookup_reverse6(xt_net(par), skb, xt_in(par),
+       return rpfilter_lookup_reverse6(xt_net(par), skb,
+                                       l3mdev_master_ifindex_rcu(xt_in(par)),
                                        info->flags) ^ invert;
 }

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

[linmiaohe]

On 2019/5/13 17:42, Pablo Neira Ayuso wrote:
> On Thu, Apr 25, 2019 at 09:43:53PM +0800, linmiaohe wrote:
>> 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>
>> ---
>>  net/ipv4/netfilter/ipt_rpfilter.c  |  1 +
>>  net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
> 
> Suggestion: Could you just call l3mdev_master_ifindex_rcu() when
> invoking rpfilter_lookup_reverse6() ?
> 
> diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> index c3c6b09acdc4..ce64ff5d6fb6 100644
> --- a/net/ipv6/netfilter/ip6t_rpfilter.c
> +++ b/net/ipv6/netfilter/ip6t_rpfilter.c
> @@ -101,7 +101,8 @@ static bool rpfilter_mt(const struct sk_buff *skb,
> struct xt_action_param *par)
>         if (unlikely(saddrtype == IPV6_ADDR_ANY))
>                 return true ^ invert; /* not routable: forward path will drop it */
>  
> -       return rpfilter_lookup_reverse6(xt_net(par), skb, xt_in(par),
> +       return rpfilter_lookup_reverse6(xt_net(par), skb,
> +                                       l3mdev_master_ifindex_rcu(xt_in(par)),
>                                         info->flags) ^ invert;
>  }
> 
> .
>     rpfilter_lookup_reverse6 requests struct net_device *dev as third argument, so
what you really mean is this ?
 diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
 index c3c6b09acdc4..ce64ff5d6fb6 100644
 --- a/net/ipv6/netfilter/ip6t_rpfilter.c
 +++ b/net/ipv6/netfilter/ip6t_rpfilter.c
 @@ -101,7 +101,8 @@ static bool rpfilter_mt(const struct sk_buff *skb,
 struct xt_action_param *par)
         if (unlikely(saddrtype == IPV6_ADDR_ANY))
                 return true ^ invert; /* not routable: forward path will drop it */

 -       return rpfilter_lookup_reverse6(xt_net(par), skb, xt_in(par),
 +       return rpfilter_lookup_reverse6(xt_net(par), skb,
 +                                       l3mdev_master_dev_rcu(xt_in(par)) ? : xt_in(par),
                                         info->flags) ^ invert;
  }
    I'am sorry but I tested this. It doesn't work. When flags with XT_RPFILTER_LOOSE set,
we need set fl6.flowi6_oif to complete fib lookup in an l3mdev domain. And we need
enslaved network device to compute rpfilter rather than l3 master device.
    Many thanks for your suggestion.
    Best regards.

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

[linmiaohe]

Friendly ping.

-----邮件原件-----
发件人: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] 代表 linmiaohe
发送时间: 2019年4月25日 21:44
收件人: pablo@netfilter.org; kadlec@blackhole.kfki.hu; fw@strlen.de; davem@davemloft.net; kuznet@ms2.inr.ac.ru; yoshfuji@linux-ipv6.org; netfilter-devel@vger.kernel.org; coreteam@netfilter.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dsahern@gmail.com
抄送: Mingfangsen <mingfangsen@huawei.com>
主题: [PATCH v3] net: netfilter: Fix rpfilter dropping vrf packets by mistake

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>
---
 net/ipv4/netfilter/ipt_rpfilter.c  |  1 +  net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
index 0b10d8812828..6e07cd0ecbec 100644
--- 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), info->flags) ^ invert;  } diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
index c3c6b09acdc4..a28c81322148 100644
--- 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;
+	}
+
 	if (rt->rt6i_idev->dev == dev || (flags & XT_RPFILTER_LOOSE))
 		ret = true;
  out:
--
2.19.1

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

[Pablo Neira Ayuso]

On Thu, Apr 25, 2019 at 09:43:53PM +0800, linmiaohe wrote:
> 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>
> ---
>  net/ipv4/netfilter/ipt_rpfilter.c  |  1 +
>  net/ipv6/netfilter/ip6t_rpfilter.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
> index 0b10d8812828..6e07cd0ecbec 100644
> --- 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), info->flags) ^ invert;
>  }
> diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> index c3c6b09acdc4..a28c81322148 100644
> --- 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!