| Message ID | 212e4feb-39de-2627-9948-bbb117ff4d4e@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3] net: netfilter: Fix rpfilter dropping vrf packets by mistake | expand |
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>
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; }
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.
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!
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: