Re: [RFC net-next 1/2] net: allow user to set IPv6 nexthop for IPv4 route

Andy Gospodarek
2015-03-27T00:27:46Z

On Fri, Mar 27, 2015 at 01:12:36AM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Thu, 26 Mar 2015, Andy Gospodarek wrote:
>
> > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> > index 66c1e4f..7de2924 100644
> > --- a/net/ipv4/fib_semantics.c
> > +++ b/net/ipv4/fib_semantics.c
> > @@ -1033,6 +1041,9 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
> > if (fi->fib_nh->nh_oif &&
> > nla_put_u32(skb, RTA_OIF, fi->fib_nh->nh_oif))
> > goto nla_put_failure;
> > + if (!ipv6_addr_any(&fi->fib_nh->nh_gw6) &&
> > + nla_put(skb, RTA_GATEWAY, 16, &fi->fib_nh->nh_gw6))
>
> RTA_GATEWAY6
>
> > + goto nla_put_failure;
> > #ifdef CONFIG_IP_ROUTE_CLASSID
> > if (fi->fib_nh[0].nh_tclassid &&
> > nla_put_u32(skb, RTA_FLOW, fi->fib_nh[0].nh_tclassid))
> > @@ -1060,6 +1071,9 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
> > if (nh->nh_gw &&
> > nla_put_be32(skb, RTA_GATEWAY, nh->nh_gw))
> > goto nla_put_failure;
> > + if (!ipv6_addr_any(&nh->nh_gw6) &&
> > + nla_put(skb, RTA_GATEWAY, 16, &nh->nh_gw6))
>
> RTA_GATEWAY6
>

Thanks for both of these, my original patch overloaded RTA_GATEWAY and I
missed these in the conversion.

> > @@ -193,10 +196,27 @@ static inline int ip_finish_output2(struct sk_buff *skb)
> > }
> >
> > rcu_read_lock_bh();
> > - nexthop = (__force u32) rt_nexthop(rt, ip_hdr(skb)->daddr);
> > - neigh = __ipv4_neigh_lookup_noref(dev, nexthop);
> > - if (unlikely(!neigh))
> > - neigh = __neigh_create(&arp_tbl, &nexthop, dev, false);
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + /* If there is an ipv6 gateway specified, use it */
> > + if (!rt->rt_gateway && !ipv6_addr_any(&rt->rt_gateway6)) {
>
> DST_NOCACHE routes can fill rt_gateway even when
> nh_gw=0 (rt_set_nexthop), so above check can be:
>
> if (rt->rt_uses_gateway && !ipv6_addr_any(&rt->rt_gateway6)) {
>
> Not sure, some places may prefer to see rt_uses_gateway=1
> for v4 and rt_uses_gateway=2 for v6 nexthop. Then above check
> can be just 'if (rt->rt_uses_gateway == 2) {'.
>

OK, thanks.

> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index be8703d..c654b41 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -1400,6 +1400,10 @@ static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
> > rt->rt_gateway = nh->nh_gw;
> > rt->rt_uses_gateway = 1;
> > }
> > + if (!ipv6_addr_any(&nh->nh_gw6)) {
> > + memcpy(&rt->rt_gateway6, &nh->nh_gw6, sizeof(struct in6_addr));
> > + rt->rt_uses_gateway = 1;
> > + }
> > dst_init_metrics(&rt->dst, fi->fib_metrics, true);
> > #ifdef CONFIG_IP_ROUTE_CLASSID
> > rt->dst.tclassid = nh->nh_tclassid;
> > @@ -1417,6 +1421,10 @@ static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
> > rt->dst.flags |= DST_NOCACHE;
> > if (!rt->rt_gateway)
> > rt->rt_gateway = daddr;
> > + if (ipv6_addr_any(&rt->rt_gateway6)) {
> > + memcpy(&rt->rt_gateway6, &nh->nh_gw6, sizeof(struct in6_addr));
> > + rt->rt_uses_gateway = 1;
> > + }
>
> Above hunk is not needed, rt_gateway6 is set above.
>

Excellent, I will drop that too.

> > rt_add_uncached_list(rt);
> > }
> > } else
> > @@ -1488,6 +1496,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
> > rth->rt_pmtu = 0;
> > rth->rt_gateway = 0;
> > rth->rt_uses_gateway = 0;
> > + memset(&rth->rt_gateway6, 0, sizeof(struct in6_addr));
>
> We can remove such initializations if
> rt_uses_gateway=2 is used as v6 indication, for example:
>
> rt_uses_gateway rt_gateway nh_gw nh_gw6 DST_NOCACHE What
> ===============================================================================
> 0 0 0/LOCAL N N Non-gatewayed, cached
> 0 DADDR 0/LOCAL N Y Non-gatewayed, not cached
> 2 0 0 GW6 N Gatewayed, cached
> 2 DADDR 0 GW6 Y Gatewayed, not cached
> 1 GW GW N Y/N Gatewayed
>
>
> May be more places need to be changed:
>
> - fib_check_nh: validate v6 address type, set nh_dev,
> allow only nh->nh_scope == RT_SCOPE_LINK (I assume
> we do not plan to use local v6 address in nh_gw6).
> - nh_comp: needs nh_gw6 comparison
> - fib_create_info: more checks are needed around
> 'if (cfg->fc_scope == RT_SCOPE_HOST) {' check
>
> There are other places that use rt_uses_gateway
> and rt_gateway.
>
> Regards
>

Awesome. Thanks for the thorough review. I'll check these bits out and
see what else needs to be done for v1 of this series.