Message ID | 52242CE1.5050501@cn.fujitsu.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote: > +static struct rt6_info *__ip6_route_redirect(struct net *net, > + struct fib6_table *table, > + struct flowi6 *fl6, > + int flags) > +{ > + struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6; > + struct rt6_info *rt; > + struct fib6_node *fn; > + > + /* Get the "current" route for this destination and > + * check if the redirect has come from approriate router. > + * > + * RFC 4861 specifies that redirects should only be > + * accepted if they come from the nexthop to the target. > + * Due to the way the routes are chosen, this notion > + * is a bit fuzzy and one might need to check all possible > + * routes. > + */ > + > + read_lock_bh(&table->tb6_lock); > + fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr); > +restart: > + for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { > + if (rt6_check_expired(rt)) > + continue; > + if (rt->dst.error) > + continue; Sorry, I should have been more clear what I meant with failing early: I considered a setup like this: ip -6 r a default nexthop via fe80::1 dev eth0 ip -6 r a prohibit 2002:1::/64 If the kernel receives a redirect for a destination e.g. 2002:1::1 we would backtrack above the prohibit rule and return the dst of the default route and would insert a new cached route which could circumvent the prohibit rule. We have to try to lock down the tree below 2002:1::/64 in that case. A possible solution for that would be to do something like this: /* We don't accept a redirect in case a more specific route is * installed with dst.error and stop backtracking. */ if (rt->dst.error) break; Either we have to replace the rt with net->ipv6.ip6_null_entry in that case or check dst->error before calling rt6_do_redirect below. > + if (!(rt->rt6i_flags & RTF_GATEWAY)) > + continue; > + if (fl6->flowi6_oif != rt->dst.dev->ifindex) > + continue; > + if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway)) > + continue; > + break; > + } > + > + if (!rt) > + rt = net->ipv6.ip6_null_entry; > + BACKTRACK(net, &fl6->saddr); > +out: > + dst_hold(&rt->dst); > + > + read_unlock_bh(&table->tb6_lock); > + > + return rt; > +}; > > [...] > > @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark) > fl6.saddr = iph->saddr; > fl6.flowlabel = ip6_flowinfo(iph); > > - dst = ip6_route_output(net, NULL, &fl6); > - if (!dst->error) > - rt6_do_redirect(dst, NULL, skb); > + dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr); > + rt6_do_redirect(dst, NULL, skb); > dst_release(dst); > } > EXPORT_SYMBOL_GPL(ip6_redirect); > @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif, > fl6.daddr = msg->dest; > fl6.saddr = iph->daddr; > > - dst = ip6_route_output(net, NULL, &fl6); > - if (!dst->error) > - rt6_do_redirect(dst, NULL, skb); > + dst = ip6_route_redirect(net, &fl6, &iph->saddr); > + rt6_do_redirect(dst, NULL, skb); > dst_release(dst); > } Btw. I still think it should be possible to eliminate ip6_redirect_no_header: We could always use ip6_redirect_no_header and use the data of the redirected header option just for finding the socket to be notified. We can do the whole verification and route updating in ndisc layer and then just call into icmpv6 layer if upper protocols need a notification of the redirect. But that should go into another patch. ;) Thanks, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2013年09月03日 03:50, Hannes Frederic Sowa 写道: > On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote: >> +static struct rt6_info *__ip6_route_redirect(struct net *net, >> + struct fib6_table *table, >> + struct flowi6 *fl6, >> + int flags) >> +{ >> + struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6; >> + struct rt6_info *rt; >> + struct fib6_node *fn; >> + >> + /* Get the "current" route for this destination and >> + * check if the redirect has come from approriate router. >> + * >> + * RFC 4861 specifies that redirects should only be >> + * accepted if they come from the nexthop to the target. >> + * Due to the way the routes are chosen, this notion >> + * is a bit fuzzy and one might need to check all possible >> + * routes. >> + */ >> + >> + read_lock_bh(&table->tb6_lock); >> + fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr); >> +restart: >> + for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { >> + if (rt6_check_expired(rt)) >> + continue; >> + if (rt->dst.error) >> + continue; > > Sorry, I should have been more clear what I meant with failing early: > > I considered a setup like this: > > ip -6 r a default nexthop via fe80::1 dev eth0 > ip -6 r a prohibit 2002:1::/64 > > If the kernel receives a redirect for a destination e.g. 2002:1::1 we > would backtrack above the prohibit rule and return the dst of the default > route and would insert a new cached route which could circumvent the > prohibit rule. We have to try to lock down the tree below 2002:1::/64 > in that case. A possible solution for that would be to do something > like this: > > /* We don't accept a redirect in case a more specific route is > * installed with dst.error and stop backtracking. > */ > if (rt->dst.error) > break; > > Either we have to replace the rt with net->ipv6.ip6_null_entry in that > case or check dst->error before calling rt6_do_redirect below. > Thanks for you comment, i understand what you mean. >> + if (!(rt->rt6i_flags & RTF_GATEWAY)) >> + continue; >> + if (fl6->flowi6_oif != rt->dst.dev->ifindex) >> + continue; >> + if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway)) >> + continue; >> + break; >> + } >> + >> + if (!rt) >> + rt = net->ipv6.ip6_null_entry; >> + BACKTRACK(net, &fl6->saddr); >> +out: >> + dst_hold(&rt->dst); >> + >> + read_unlock_bh(&table->tb6_lock); >> + >> + return rt; >> +}; >> >> [...] >> >> @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark) >> fl6.saddr = iph->saddr; >> fl6.flowlabel = ip6_flowinfo(iph); >> >> - dst = ip6_route_output(net, NULL, &fl6); >> - if (!dst->error) >> - rt6_do_redirect(dst, NULL, skb); >> + dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr); >> + rt6_do_redirect(dst, NULL, skb); >> dst_release(dst); >> } >> EXPORT_SYMBOL_GPL(ip6_redirect); >> @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif, >> fl6.daddr = msg->dest; >> fl6.saddr = iph->daddr; >> >> - dst = ip6_route_output(net, NULL, &fl6); >> - if (!dst->error) >> - rt6_do_redirect(dst, NULL, skb); >> + dst = ip6_route_redirect(net, &fl6, &iph->saddr); >> + rt6_do_redirect(dst, NULL, skb); >> dst_release(dst); >> } > > Btw. I still think it should be possible to eliminate > ip6_redirect_no_header: > > We could always use ip6_redirect_no_header and use the data of the redirected > header option just for finding the socket to be notified. We can do the whole > verification and route updating in ndisc layer and then just call into icmpv6 > layer if upper protocols need a notification of the redirect. But that should > go into another patch. ;) > I think this is good, but i have a question below: if the socket type is connection-based, the dst information is stored in related sock struct, so there is no need to look up the route for redirect in ip6_redirect or ip6_redirect_no_header, in this case, we do the verification and route updating in the upper protocols' err_handler is better. How do you think of this? Thanks, Duan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote: > > Btw. I still think it should be possible to eliminate > > ip6_redirect_no_header: > > > > We could always use ip6_redirect_no_header and use the data of the redirected > > header option just for finding the socket to be notified. We can do the whole > > verification and route updating in ndisc layer and then just call into icmpv6 > > layer if upper protocols need a notification of the redirect. But that should > > go into another patch. ;) > > > > I think this is good, but i have a question below: > > if the socket type is connection-based, the dst information is stored in related > sock struct, so there is no need to look up the route for redirect in ip6_redirect > or ip6_redirect_no_header, in this case, we do the verification and route > updating in the upper protocols' err_handler is better. > > How do you think of this? This should not be a problem, because every cached dst should be validated with ip6_dst_check before it is used. It uses the fib6_node serial number which is incremented for all fib6_nodes on the path to the new installed node by fib6_add_1. So we are safe here. Btw. this is the same logic redirects get currently picked up, too. Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2013年09月04日 03:17, Hannes Frederic Sowa 写道: > On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote: >>> Btw. I still think it should be possible to eliminate >>> ip6_redirect_no_header: >>> >>> We could always use ip6_redirect_no_header and use the data of the redirected >>> header option just for finding the socket to be notified. We can do the whole >>> verification and route updating in ndisc layer and then just call into icmpv6 >>> layer if upper protocols need a notification of the redirect. But that should >>> go into another patch. ;) >>> >> >> I think this is good, but i have a question below: >> >> if the socket type is connection-based, the dst information is stored in related >> sock struct, so there is no need to look up the route for redirect in ip6_redirect >> or ip6_redirect_no_header, in this case, we do the verification and route >> updating in the upper protocols' err_handler is better. >> >> How do you think of this? > > This should not be a problem, because every cached dst should be validated > with ip6_dst_check before it is used. It uses the fib6_node serial number > which is incremented for all fib6_nodes on the path to the new installed > node by fib6_add_1. So we are safe here. > > Btw. this is the same logic redirects get currently picked up, too. > Thanks for you answer, but i still have some questions on dealing with redirect in ip4ip6_err() and ipip6_err(), and i need some time to learn more about them. So i only send one patch to fix the bug. Please forgive me is a newbie.:) Thanks, Duan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 04, 2013 at 08:06:11PM +0800, Duan Jiong wrote: > 于 2013年09月04日 03:17, Hannes Frederic Sowa 写道: > > On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote: > >>> Btw. I still think it should be possible to eliminate > >>> ip6_redirect_no_header: > >>> > >>> We could always use ip6_redirect_no_header and use the data of the redirected > >>> header option just for finding the socket to be notified. We can do the whole > >>> verification and route updating in ndisc layer and then just call into icmpv6 > >>> layer if upper protocols need a notification of the redirect. But that should > >>> go into another patch. ;) > >>> > >> > >> I think this is good, but i have a question below: > >> > >> if the socket type is connection-based, the dst information is stored in related > >> sock struct, so there is no need to look up the route for redirect in ip6_redirect > >> or ip6_redirect_no_header, in this case, we do the verification and route > >> updating in the upper protocols' err_handler is better. > >> > >> How do you think of this? > > > > This should not be a problem, because every cached dst should be validated > > with ip6_dst_check before it is used. It uses the fib6_node serial number > > which is incremented for all fib6_nodes on the path to the new installed > > node by fib6_add_1. So we are safe here. > > > > Btw. this is the same logic redirects get currently picked up, too. > > > > Thanks for you answer, but i still have some questions on dealing with redirect > in ip4ip6_err() and ipip6_err(), and i need some time to learn more about them. > So i only send one patch to fix the bug. Coverity discovered that the redirect code in ip6_tunnel.c is logically dead, which is correct: 639 } 640 if (rel_type == ICMP_REDIRECT) 641 skb_dst(skb2)->ops->redirect(skb_dst(skb2), NULL, skb2); 642 643 icmp_send(skb2, rel_type, rel_code, htonl(rel_info)); rel_type will never be ICMP_REDIRECT in line 640 because of the updates to rel_type in ip6_tnl_err. My guess is that we need to move the call to ->redirect to ip6_tnl_err and afterwards set rel_msg to 0 or we factor out the calls to ->redirect into the ndisc layer. I hope this clears up some confusion you had in ip6ip6_err. Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/ipv6/ah6.c b/net/ipv6/ah6.c index bb02e17..73784c3 100644 --- a/net/ipv6/ah6.c +++ b/net/ipv6/ah6.c @@ -628,7 +628,7 @@ static void ah6_err(struct sk_buff *skb, struct inet6_skb_parm *opt, return; if (type == NDISC_REDIRECT) - ip6_redirect(skb, net, 0, 0); + ip6_redirect(skb, net, skb->dev->ifindex, 0); else ip6_update_pmtu(skb, net, info, 0, 0); xfrm_state_put(x); diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c index aeac0dc..d3618a7 100644 --- a/net/ipv6/esp6.c +++ b/net/ipv6/esp6.c @@ -447,7 +447,7 @@ static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt, return; if (type == NDISC_REDIRECT) - ip6_redirect(skb, net, 0, 0); + ip6_redirect(skb, net, skb->dev->ifindex, 0); else ip6_update_pmtu(skb, net, info, 0, 0); xfrm_state_put(x); diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index 7cfc8d2..73681c2 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -92,7 +92,7 @@ static void icmpv6_err(struct sk_buff *skb, struct inet6_skb_parm *opt, if (type == ICMPV6_PKT_TOOBIG) ip6_update_pmtu(skb, net, info, 0, 0); else if (type == NDISC_REDIRECT) - ip6_redirect(skb, net, 0, 0); + ip6_redirect(skb, net, skb->dev->ifindex, 0); if (!(type & ICMPV6_INFOMSG_MASK)) if (icmp6->icmp6_type == ICMPV6_ECHO_REQUEST) diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c index 7af5aee..5636a91 100644 --- a/net/ipv6/ipcomp6.c +++ b/net/ipv6/ipcomp6.c @@ -76,7 +76,7 @@ static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt, return; if (type == NDISC_REDIRECT) - ip6_redirect(skb, net, 0, 0); + ip6_redirect(skb, net, skb->dev->ifindex, 0); else ip6_update_pmtu(skb, net, info, 0, 0); xfrm_state_put(x); diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c index 04d31c2..70abece 100644 --- a/net/ipv6/ndisc.c +++ b/net/ipv6/ndisc.c @@ -1370,7 +1370,8 @@ static void ndisc_redirect_rcv(struct sk_buff *skb) return; if (!ndopts.nd_opts_rh) { - ip6_redirect_no_header(skb, dev_net(skb->dev), 0, 0); + ip6_redirect_no_header(skb, dev_net(skb->dev), + skb->dev->ifindex, 0); return; } diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 8d9a93e..a01337f 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -1157,6 +1157,73 @@ void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu) } EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu); +/* Handle redirects */ +struct ip6rd_flowi { + struct flowi6 fl6; + struct in6_addr gateway; +}; + +static struct rt6_info *__ip6_route_redirect(struct net *net, + struct fib6_table *table, + struct flowi6 *fl6, + int flags) +{ + struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6; + struct rt6_info *rt; + struct fib6_node *fn; + + /* Get the "current" route for this destination and + * check if the redirect has come from approriate router. + * + * RFC 4861 specifies that redirects should only be + * accepted if they come from the nexthop to the target. + * Due to the way the routes are chosen, this notion + * is a bit fuzzy and one might need to check all possible + * routes. + */ + + read_lock_bh(&table->tb6_lock); + fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr); +restart: + for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { + if (rt6_check_expired(rt)) + continue; + if (rt->dst.error) + continue; + if (!(rt->rt6i_flags & RTF_GATEWAY)) + continue; + if (fl6->flowi6_oif != rt->dst.dev->ifindex) + continue; + if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway)) + continue; + break; + } + + if (!rt) + rt = net->ipv6.ip6_null_entry; + BACKTRACK(net, &fl6->saddr); +out: + dst_hold(&rt->dst); + + read_unlock_bh(&table->tb6_lock); + + return rt; +}; + +static struct dst_entry *ip6_route_redirect(struct net *net, + const struct flowi6 *fl6, + const struct in6_addr *gateway) +{ + int flags = RT6_LOOKUP_F_HAS_SADDR; + struct ip6rd_flowi rdfl; + + rdfl.fl6 = *fl6; + rdfl.gateway = *gateway; + + return fib6_rule_lookup(net, &rdfl.fl6, + flags, __ip6_route_redirect); +} + void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark) { const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data; @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark) fl6.saddr = iph->saddr; fl6.flowlabel = ip6_flowinfo(iph); - dst = ip6_route_output(net, NULL, &fl6); - if (!dst->error) - rt6_do_redirect(dst, NULL, skb); + dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr); + rt6_do_redirect(dst, NULL, skb); dst_release(dst); } EXPORT_SYMBOL_GPL(ip6_redirect); @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif, fl6.daddr = msg->dest; fl6.saddr = iph->daddr; - dst = ip6_route_output(net, NULL, &fl6); - if (!dst->error) - rt6_do_redirect(dst, NULL, skb); + dst = ip6_route_redirect(net, &fl6, &iph->saddr); + rt6_do_redirect(dst, NULL, skb); dst_release(dst); }