| Message ID | 1368445651-6822-1-git-send-email-fw@strlen.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi Florian, On Mon, May 13, 2013 at 01:47:31PM +0200, Florian Westphal wrote: [...] > AFAIU there are two possible solutions: > > a), extend struct nf_afinfo to also register ipv6_chk_addr(), OR > b), revert the commit that moved ipt_addrtype to xt_addrtype, > and keep the ipv6 code in ip6t_addrtype. > > IMO, the latter seems to be preferable, but would be more intrusive. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > As explained earlier, I don't like this approach; IMO the proper solution > is to split xt_addrinfo into ipt_addrinfo and ip6t_addrinfo. > The only downside is that it will create a bit of code duplication due > to checkentry() functions, but it avoids adding is_local_addr hook > for the sole purpose of fixing ipv6 xt_addrinfo. ipv6_find_hdr was also moved from ip6tables to ipv6 core code recently. Now we got a hard dependency on ipv6 if Hans' HMARK is used as well. So we need another hook for it. Again, that function is pretty specific of IPv6. So I think that we can add a new struct nf_afinfo_ipv6 to keep IPv6-only hooks like this and the one for ipv6_find. Cong Wang also reported some similar problems when IPv6 dependencies that we could also fix by populating that structure with more hooks. I don't like putting this into nf_afinfo either, since it's specific of IPv6, but I want a small fix that fulfill the -stable rules. It will take some time until people get the fix for xt_addrtype IPv6 if we make it the nice way. Seems like merge ipt and ip6t module is bringing us more problems that expected. [...] > I can pass a patch for this to davem one net-next is open if > you agree with this patch. I'd like to get this into net asap, it is fixing xt_addrtype for the IPv6 case, then pass it to -stable. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > ipv6_find_hdr was also moved from ip6tables to ipv6 core code > recently. Now we got a hard dependency on ipv6 if Hans' HMARK is used > as well. So we need another hook for it. Again, that function is > pretty specific of IPv6. So I think that we can add a new struct > nf_afinfo_ipv6 to keep IPv6-only hooks like this and the one for > ipv6_find. Alright, i'll re-work this patch into a series, first adding such a new struct. We can then fix other dependency crap later as time permits. > I don't like putting this into nf_afinfo either, since it's specific > of IPv6, but I want a small fix that fulfill the -stable rules. It > will take some time until people get the fix for xt_addrtype IPv6 if > we make it the nice way. True, although this isn't a regression. > Seems like merge ipt and ip6t module is bringing us more problems that > expected. Yes, and I think that separating all of those again is not realistic, so a new ipv6 specific hook struct seems like the best shot. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h index 0060fde..66f9af0 100644 --- a/include/linux/netfilter.h +++ b/include/linux/netfilter.h @@ -228,6 +228,9 @@ struct nf_afinfo { struct nf_queue_entry *entry); int (*reroute)(struct sk_buff *skb, const struct nf_queue_entry *entry); + int (*is_local_addr)(struct net *net, const void *addr, + const struct net_device *dev, + bool strict); int route_key_size; }; diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c index c3e0ade..29f2ea9 100644 --- a/net/ipv4/netfilter.c +++ b/net/ipv4/netfilter.c @@ -183,6 +183,17 @@ static int nf_ip_route(struct net *net, struct dst_entry **dst, return 0; } +static bool +nf_ip_local_address(struct net *net, + const void *addr, + const struct net_device *dev, + bool strict) +{ + /* only ipv6 version is used at this time */ + WARN_ON_ONCE(1); + return false; +} + static const struct nf_afinfo nf_ip_afinfo = { .family = AF_INET, .checksum = nf_ip_checksum, @@ -190,6 +201,7 @@ static const struct nf_afinfo nf_ip_afinfo = { .route = nf_ip_route, .saveroute = nf_ip_saveroute, .reroute = nf_ip_reroute, + .is_local_addr = nf_ip_local_address, .route_key_size = sizeof(struct ip_rt_info), }; diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c index 72836f4..53d009d 100644 --- a/net/ipv6/netfilter.c +++ b/net/ipv6/netfilter.c @@ -10,6 +10,7 @@ #include <linux/netfilter.h> #include <linux/netfilter_ipv6.h> #include <linux/export.h> +#include <net/addrconf.h> #include <net/dst.h> #include <net/ipv6.h> #include <net/ip6_route.h> @@ -186,6 +187,15 @@ static __sum16 nf_ip6_checksum_partial(struct sk_buff *skb, unsigned int hook, return csum; }; +static bool +nf_ip6_local_address(struct net *net, + const void *addr, + const struct net_device *dev, + bool strict) +{ + return ipv6_chk_addr(net, addr, dev, strict) != 0; +} + static const struct nf_afinfo nf_ip6_afinfo = { .family = AF_INET6, .checksum = nf_ip6_checksum, @@ -193,6 +203,7 @@ static const struct nf_afinfo nf_ip6_afinfo = { .route = nf_ip6_route, .saveroute = nf_ip6_saveroute, .reroute = nf_ip6_reroute, + .is_local_addr = nf_ip6_local_address, .route_key_size = sizeof(struct ip6_rt_info), }; diff --git a/net/netfilter/xt_addrtype.c b/net/netfilter/xt_addrtype.c index 49c5ff7..f4f91b4 100644 --- a/net/netfilter/xt_addrtype.c +++ b/net/netfilter/xt_addrtype.c @@ -33,28 +33,29 @@ MODULE_ALIAS("ip6t_addrtype"); #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) static u32 match_lookup_rt6(struct net *net, const struct net_device *dev, - const struct in6_addr *addr) + const struct in6_addr *addr, u16 mask) { const struct nf_afinfo *afinfo; - struct flowi6 flow; + struct flowi6 flow = { .daddr = *addr }; struct rt6_info *rt; - u32 ret; + u32 ret = 0; int route_err; - memset(&flow, 0, sizeof(flow)); - flow.daddr = *addr; if (dev) flow.flowi6_oif = dev->ifindex; rcu_read_lock(); afinfo = nf_get_afinfo(NFPROTO_IPV6); - if (afinfo != NULL) + if (afinfo != NULL) { + if (dev && (mask & XT_ADDRTYPE_LOCAL) && + afinfo->is_local_addr(net, addr, dev, true)) + ret = XT_ADDRTYPE_LOCAL; route_err = afinfo->route(net, (struct dst_entry **)&rt, - flowi6_to_flowi(&flow), !!dev); - else + flowi6_to_flowi(&flow), false); + } else { route_err = 1; - + } rcu_read_unlock(); if (route_err) @@ -62,10 +63,8 @@ static u32 match_lookup_rt6(struct net *net, const struct net_device *dev, if (rt->rt6i_flags & RTF_REJECT) ret = XT_ADDRTYPE_UNREACHABLE; - else - ret = 0; - if (rt->rt6i_flags & RTF_LOCAL) + if (dev == NULL && rt->rt6i_flags & RTF_LOCAL) ret |= XT_ADDRTYPE_LOCAL; if (rt->rt6i_flags & RTF_ANYCAST) ret |= XT_ADDRTYPE_ANYCAST; @@ -90,7 +89,7 @@ static bool match_type6(struct net *net, const struct net_device *dev, if ((XT_ADDRTYPE_LOCAL | XT_ADDRTYPE_ANYCAST | XT_ADDRTYPE_UNREACHABLE) & mask) - return !!(mask & match_lookup_rt6(net, dev, addr)); + return !!(mask & match_lookup_rt6(net, dev, addr, mask)); return true; }
/quote https://bugzilla.netfilter.org/show_bug.cgi?id=812 : When I tried to use in the nat/PREROUTING it messes up the routing cache even if the rule didn't matched at all. [..] If I remove the --limit-iface-in from the non-working scenario, so just use the -m addrtype --dst-type LOCAL it works! /unquote This happens when LOCAL type matching is requested with --limit-iface-in, and the default route is via the interface the packet we test arrived on. Because xt_addrtype uses ip6_route_output, the ipv6 routing implementation creates an unwanted cached entry, and the packet won't make it to the real/expected destination. Silently ignoring --limit-iface-in makes the routing work but it breaks rule matching (--dst-type LOCAL with limit-iface-in is supposed to only match if the dst address is configured on the incoming interface; without --limit-iface-in it will match if the address is reachable via lo). AFAIU there are two possible solutions: a), extend struct nf_afinfo to also register ipv6_chk_addr(), OR b), revert the commit that moved ipt_addrtype to xt_addrtype, and keep the ipv6 code in ip6t_addrtype. IMO, the latter seems to be preferable, but would be more intrusive. Signed-off-by: Florian Westphal <fw@strlen.de> --- As explained earlier, I don't like this approach; IMO the proper solution is to split xt_addrinfo into ipt_addrinfo and ip6t_addrinfo. The only downside is that it will create a bit of code duplication due to checkentry() functions, but it avoids adding is_local_addr hook for the sole purpose of fixing ipv6 xt_addrinfo. Árpád, it would be nice if you could test if this patch indeed fixes your problem. Pablo, please don't apply yet. the patch causes net/ipv6/netfilter.c:197:2: warning: passing argument 3 of 'ipv6_chk_addr' discards qualifiers from pointer target type include/net/addrconf.h:66:14: note: expected 'struct net_device *' but argument is of type 'const struct net_device *' I can pass a patch for this to davem one net-next is open if you agree with this patch. include/linux/netfilter.h | 3 +++ net/ipv4/netfilter.c | 12 ++++++++++++ net/ipv6/netfilter.c | 11 +++++++++++ net/netfilter/xt_addrtype.c | 25 ++++++++++++------------- 4 files changed, 38 insertions(+), 13 deletions(-)