Re: [PATCH for-next] RDMA/rxe: Fix ib_device reference counting (again)

From: Leon Romanovsky <leon@kernel.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: "Pearson, Robert B" <robert.pearson2@hpe.com>,
	Bob Pearson <rpearsonhpe@gmail.com>,
	"zyjzyj2000@gmail.com" <zyjzyj2000@gmail.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"Zago, Frank" <frank.zago@hpe.com>
Subject: Re: [PATCH for-next] RDMA/rxe: Fix ib_device reference counting (again)
Date: Tue, 2 Mar 2021 10:11:46 +0200	[thread overview]
Message-ID: <YD3zQjVgoTpiQlei@unreal> (raw)
In-Reply-To: <20210301182711.GZ4247@nvidia.com>

On Mon, Mar 01, 2021 at 02:27:11PM -0400, Jason Gunthorpe wrote:
> On Mon, Mar 01, 2021 at 06:20:06PM +0000, Pearson, Robert B wrote:
> > > On Mon, Mar 01, 2021 at 10:54:21AM -0600, Bob Pearson wrote:
> >
> > >> I agree that ib_device_get/put is attempting to solve a problem that
> > >> it not really very critical since ib_device is very unlikely to be
> > >> shut down in the middle of a data transfer. The driver never worried about this for years.
> > >> But now that it's been put on the table it should be done right. A
> > >> data packet arriving is completely independent of the verbs API which
> > >> *could* delete all the QPs and shut down the HCA while it was
> > >> wondering around the universe or worse yet while the packet is being processed.
> >
> > > If driver shutdown can guarentee that all pointers involved in
> > > multicast are revoked before shutdown can finish then you don't
> > > need this refcounting.
> >
> > > It was only brought up because the API that returns the ib_device
> > > from the netdev requires the refcounts as it is general purpose
> >
> > Unfortunately what you ask for is exactly what the refcounting code
> > accomplishes and I don't see a simpler way to get there.  This also
> > applies to the non-multicast packets as well but all the debate has
> > been about the code in rxe_rcv_mcast_pkt() because it is more
> > blatant there or because I haven't been able to explain how it works
> > well enough.
>
> Usually in the netstack land the shutdown of the device flushes all
> this parallel work out so all the dataplane can happily ignore all
> these details.
>
> I'm not so clear on all these details and how they apply to rxe of
> course. You'd have to look at the full lifecycle of this skb and show
> that the kfree_skb happens only before any unregistration finishes.
>
> Most likely there are other bugs if the unregistration can pass while
> the skb is still out there.
>
> But, I'm not clear on how any of this works in rxe, this is just a
> general remark on how things should ideally work.

+1, I have same understanding and expect SKB to be flushed and new SKB
are prevented from entering ib_device if it is going under destroy.

Thanks

>
> Jason