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@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
