[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!

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

[linmiaohe]

On 2019/6/18 23:58, 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>
>> ---
>> --- 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 your reply. I will try to explain this in more detail.
    Vrf device will pass through netfilter hook twice. One with skb->dev=l3mdev
Slave device and another one with skb->dev=l3mdev master deivce.
    If a device is an l3mdev,  l3mdev_master_ifindex_rcu will return l3mdev
master device ifindex otherwise 0 . So for non l3mdev cases,  v4 version is
as same as the previous one. And for l3mdev cases,  flow.flowi4_oif
will be l3mdev master device ifindex, so we can do a fib lookup in l3mdev
domain as expected. Since fib_info_nh_uses_dev help us handle the case with
dev=l3mdev slave or master and  XT_RPFILTER_LOOSE do not lookup route
table, we finish v4.
    For v6 version we need to set fl6.flowi6_oif as we are supposed to lookup 
fib in l3mdev domain even in XT_RPFILTER_LOOSE mode.
    And fib result rt->rt6i_idev->dev is l3mdev slave device, we need change
dev to enslaved l3mdev device when dev passed in is l3mdev master device.
    The key is l3mdev will pass through netfilter hook twice with skb dev is l3mdev slave
and master . And we need to set flowi6_oif as fib lookup should in the l3mdev
domain.
    Thanks a lot. Have a good day!

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

[Pablo Neira Ayuso]

On Wed, Jun 19, 2019 at 09:49:04AM +0000, linmiaohe wrote:
> 
> On 2019/6/18 23:58, 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>
> >> ---
> >> --- 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 your reply. I will try to explain this in more detail.
>     Vrf device will pass through netfilter hook twice. One with skb->dev=l3mdev
> Slave device and another one with skb->dev=l3mdev master deivce.
>     If a device is an l3mdev,  l3mdev_master_ifindex_rcu will return l3mdev
> master device ifindex otherwise 0 . So for non l3mdev cases,  v4 version is
> as same as the previous one. And for l3mdev cases,  flow.flowi4_oif
> will be l3mdev master device ifindex, so we can do a fib lookup in l3mdev
> domain as expected. Since fib_info_nh_uses_dev help us handle the case with
> dev=l3mdev slave or master and  XT_RPFILTER_LOOSE do not lookup route
> table, we finish v4.
>     For v6 version we need to set fl6.flowi6_oif as we are supposed to lookup 
> fib in l3mdev domain even in XT_RPFILTER_LOOSE mode.
>     And fib result rt->rt6i_idev->dev is l3mdev slave device, we need change
> dev to enslaved l3mdev device when dev passed in is l3mdev master device.
>     The key is l3mdev will pass through netfilter hook twice with skb dev is l3mdev slave
> and master . And we need to set flowi6_oif as fib lookup should in the l3mdev
> domain.

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.

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

[linmiaohe]

On Wed, Jun 19, 2019 at 09:49:04AM +0000, linmiaohe wrote:
> 
> On 2019/6/18 23:58, 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>
> >> ---
> >> --- 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.