| Message ID | 20180225194730.30063-3-dsahern@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/ipv6: Separate data structures for FIB and data path | expand |
From: David Ahern <dsahern@gmail.com> Date: Sun, 25 Feb 2018 11:47:12 -0800 > A later patch removes rt6i_table from rt6_info. Save the ipv6 > table for a VRF in net_vrf. fib tables can not be deleted so > no reference counting or locking is required. > > Signed-off-by: David Ahern <dsahern@gmail.com> Is this change really OK all by itself? Sure, you fix up the vrf code operating on such 'rt6' objects to not dereference the ->rt6i_table. But any other code whatsoever that looks at this rt6 object (dumping, other operations in the ipv6 stack data or control plane, etc.) can legitimately expect always to see a non-NULL value here. I really don't see how this can be OK and leave your patch series properly bisectable. Thanks.
On 2/26/18 12:08 PM, David Miller wrote: > From: David Ahern <dsahern@gmail.com> > Date: Sun, 25 Feb 2018 11:47:12 -0800 > >> A later patch removes rt6i_table from rt6_info. Save the ipv6 >> table for a VRF in net_vrf. fib tables can not be deleted so >> no reference counting or locking is required. >> >> Signed-off-by: David Ahern <dsahern@gmail.com> > > Is this change really OK all by itself? should. When you look at references to rt6i_table the only one for the data path is changing sernum when inserting an exception. Exceptions are not relevant for this special VRF dst as it is used for an internal redirection before packets leave the box. > > Sure, you fix up the vrf code operating on such 'rt6' objects to > not dereference the ->rt6i_table. > > But any other code whatsoever that looks at this rt6 object (dumping, > other operations in the ipv6 stack data or control plane, etc.) can > legitimately expect always to see a non-NULL value here. > > I really don't see how this can be OK and leave your patch series > properly bisectable. > But if you want me to be extra cautious I can leave the rt6i_table setting and remove it in patch 20 (remove unneeded rt6_info elements).
From: David Ahern <dsahern@gmail.com> Date: Mon, 26 Feb 2018 13:13:24 -0700 > On 2/26/18 12:08 PM, David Miller wrote: >> From: David Ahern <dsahern@gmail.com> >> Date: Sun, 25 Feb 2018 11:47:12 -0800 >> >>> A later patch removes rt6i_table from rt6_info. Save the ipv6 >>> table for a VRF in net_vrf. fib tables can not be deleted so >>> no reference counting or locking is required. >>> >>> Signed-off-by: David Ahern <dsahern@gmail.com> >> >> Is this change really OK all by itself? > > should. When you look at references to rt6i_table the only one for the > data path is changing sernum when inserting an exception. Exceptions are > not relevant for this special VRF dst as it is used for an internal > redirection before packets leave the box. ... > But if you want me to be extra cautious I can leave the rt6i_table > setting and remove it in patch 20 (remove unneeded rt6_info elements). No, it's not necessary if it is really an isolated object like this.
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c index 9ce0182223a0..7d5407eede6c 100644 --- a/drivers/net/vrf.c +++ b/drivers/net/vrf.c @@ -48,6 +48,9 @@ static unsigned int vrf_net_id; struct net_vrf { struct rtable __rcu *rth; struct rt6_info __rcu *rt6; +#if IS_ENABLED(CONFIG_IPV6) + struct fib6_table *fib6_table; +#endif u32 tb_id; }; @@ -496,7 +499,6 @@ static int vrf_rt6_create(struct net_device *dev) int flags = DST_HOST | DST_NOPOLICY | DST_NOXFRM; struct net_vrf *vrf = netdev_priv(dev); struct net *net = dev_net(dev); - struct fib6_table *rt6i_table; struct rt6_info *rt6; int rc = -ENOMEM; @@ -504,8 +506,8 @@ static int vrf_rt6_create(struct net_device *dev) if (!ipv6_mod_enabled()) return 0; - rt6i_table = fib6_new_table(net, vrf->tb_id); - if (!rt6i_table) + vrf->fib6_table = fib6_new_table(net, vrf->tb_id); + if (!vrf->fib6_table) goto out; /* create a dst for routing packets out a VRF device */ @@ -513,7 +515,6 @@ static int vrf_rt6_create(struct net_device *dev) if (!rt6) goto out; - rt6->rt6i_table = rt6i_table; rt6->dst.output = vrf_output6; rcu_assign_pointer(vrf->rt6, rt6); @@ -944,22 +945,8 @@ static struct rt6_info *vrf_ip6_route_lookup(struct net *net, int flags) { struct net_vrf *vrf = netdev_priv(dev); - struct fib6_table *table = NULL; - struct rt6_info *rt6; - - rcu_read_lock(); - - /* fib6_table does not have a refcnt and can not be freed */ - rt6 = rcu_dereference(vrf->rt6); - if (likely(rt6)) - table = rt6->rt6i_table; - - rcu_read_unlock(); - - if (!table) - return NULL; - return ip6_pol_route(net, table, ifindex, fl6, flags); + return ip6_pol_route(net, vrf->fib6_table, ifindex, fl6, flags); } static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
A later patch removes rt6i_table from rt6_info. Save the ipv6 table for a VRF in net_vrf. fib tables can not be deleted so no reference counting or locking is required. Signed-off-by: David Ahern <dsahern@gmail.com> --- drivers/net/vrf.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)