> -----Original Message----- > From: Jason Gunthorpe > Sent: Wednesday, September 5, 2018 5:29 PM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; Leon Romanovsky > <leon@xxxxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky > <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>; > Daniel Jurgens <danielj@xxxxxxxxxxxx>; Yuval Shaia <yuval.shaia@xxxxxxxxxx> > Subject: Re: [PATCH rdma-next 01/11] RDMA/core Introduce and use > rdma_find_ndev_for_src_ip_rcu > > On Wed, Sep 05, 2018 at 04:07:48PM -0600, Parav Pandit wrote: > > > > Is taking it before calling rdma_find_ndev_for_src_ip_rcu() the right thing to > do? > > Likely because rcu_read_lock can be nested as per [1]. > > > If we do rcu_read_lock() inside rdma_find_ndev_for_src_ip_rcu(), we > > need to dev_hold and dev_put() and handle code differently between > > ipv4 and v6. > > ?? We already have dev_hold in both and we already dev_put, if we move the > read_lock it would be just to put it in the ipv6 leg of that switch.. > In rdma_translate_ip() before this patch, dev_hold/put is present only for ipv4. For ipv6 it is done under rcu. With this patch, we avoid dev_hold/put for ipv4 and follow similar approach as ipv6. > > However, dev_hold() and dev_put() doesn't guarantee that netdev fields > > won't change such as ifindex. It just ensures that netdev exist. > > That explanation would make more sense if we didn't immediately drop the rcu > after copying the if_index into a struct. > > Nothing can safely read that structure member, so why did we do it? > When we want to work out of stable net and netdev pointer in patch 11 of the series for incoming RoCE CM requests, We continue to hold the rcu and resolve the route and source addresses which invokes, rdma_set_src_addr_rcu() and copy_src_l2_addr(). This is done for stable netdev in IFF_UP status. Therefore it sync with synchronize_rcu() of the net/dev/core.c and provides the safety of stable, net and netdev. > I thought we got rid of all the ifindex stuff? bound_dev_if still exist and we continue to reduce dependency or open access to it. > Is this just a weirdness in-between patches in the series? I haven't got to look at these patches yet.. At least in this particular patch context, yes, it doesn't really protect ifindex. But it does protect addr_resolve() for nw one once the whole series is present. In other words since dev_hold/put is removed, rcu lock ensures that netdev is not getting freed while copy_addr() is going on. So instead of writing two patches that add code in fix-1 and delete it in patch-11 in same series, it is written for functionality that fixes the issue too.
