Re: [PATCH rdma-next 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

On Mon, Jan 22, 2018 at 11:38:20AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 09, 2018 at 03:58:58PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Netdev ifindex can change while performing IPv6 rt6_lookup().
> > Therefore access ifindex under rcu lock.
> > This ensures that ifindex won't change while lookup is in progress.
> >
> > Fixes: f887f2ac87c2 ("IB/cma: Validate routing of incoming requests")
> > Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >  drivers/infiniband/core/cma.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 90dead30de4c..690ed4238d72 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -1333,11 +1333,15 @@ static bool validate_ipv6_net_dev(struct net_device *net_dev,
> >  #if IS_ENABLED(CONFIG_IPV6)
> >  	const int strict = ipv6_addr_type(&dst_addr->sin6_addr) &
> >  			   IPV6_ADDR_LINKLOCAL;
> > -	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
> > -					 &src_addr->sin6_addr, net_dev->ifindex,
> > -					 strict);
> > +	struct rt6_info *rt;
> >  	bool ret;
> >
> > +	rcu_read_lock();
> > +	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
> > +			&src_addr->sin6_addr, net_dev->ifindex,
> > +			strict);
> > +	rcu_read_unlock();
>
> This doesn't make sense to me.
>
> rcu is not supposed to be used in cases where two variables can change
> concurrently.
>
> For instance if the ifindex is changed due to dev_change_net_namespace()
> then you have this:
>
> 	dev_net_set(dev, net);
> 	if (__dev_get_by_index(net, dev->ifindex))
> 		dev->ifindex = dev_new_index(net);
>
> Racing with this:
>
> 	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
> 			&src_addr->sin6_addr, net_dev->ifindex,
> 			strict);
>
> And we will get a racy incoherent result for dev_net(net_dev) and
> net_dev->ifindex.

I think that rcu_derefence over net_dev will solve the race.

>
> Same comment for the next patch too.
>
> It kind of looks to me like the locking scheme in the netdev side
> shuts down the netdev while moving it. So the RCU protected test
> should be if the netdev is DOWN ?

It is not the case for ipv6 tunnel interface, they use RCU protection
for live netdevs too.

>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]