Re: Re: [PATCH v3] iwcm: don't hold the irq disabled lock on iw_rem_ref

From: Steve Wise <larrystevenwise@gmail.com>
To: Bernard Metzler <BMT@zurich.ibm.com>
Cc: Krishnamraju Eraparaju <krishna2@chelsio.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	Jason Gunthorpe <jgg@ziepe.ca>
Subject: Re: Re: [PATCH v3] iwcm: don't hold the irq disabled lock on iw_rem_ref
Date: Wed, 11 Sep 2019 09:42:43 -0500	[thread overview]
Message-ID: <CADmRdJcCENJx==LaaJQYU_kMv5rSgD69Z6s+ubCKWjprZmPQpA@mail.gmail.com> (raw)
In-Reply-To: <OF00E4DFD9.0EEF58A6-ON00258472.0032F9AC-00258472.0034FEAA@notes.na.collabserv.com>

On Wed, Sep 11, 2019 at 4:38 AM Bernard Metzler <BMT@zurich.ibm.com> wrote:
>
> -----"Krishnamraju Eraparaju" <krishna2@chelsio.com> wrote: -----
>
> >To: "Sagi Grimberg" <sagi@grimberg.me>, "Steve Wise"
> ><larrystevenwise@gmail.com>, "Bernard Metzler" <BMT@zurich.ibm.com>
> >From: "Krishnamraju Eraparaju" <krishna2@chelsio.com>
> >Date: 09/10/2019 09:22PM
> >Cc: "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>, "Jason
> >Gunthorpe" <jgg@ziepe.ca>
> >Subject: [EXTERNAL] Re: [PATCH v3] iwcm: don't hold the irq disabled
> >lock on iw_rem_ref
> >
> >Please review the below patch, I will resubmit this in patch-series
> >after review.
> >- As kput_ref handler(siw_free_qp) uses vfree, iwcm can't call
> >  iw_rem_ref() with spinlocks held. Doing so can cause vfree() to
> >sleep
> >  with irq disabled.
> >  Two possible solutions:
> >  1)With spinlock acquired, take a copy of "cm_id_priv->qp" and
> >update
> >    it to NULL. And after releasing lock use the copied qp pointer
> >for
> >    rem_ref().
> >  2)Replacing issue causing vmalloc()/vfree to kmalloc()/kfree in SIW
> >    driver, may not be a ideal solution.
> >
> >  Solution 2 may not be ideal as allocating huge contigous memory for
> >   SQ & RQ doesn't look appropriate.
> >
> >- The structure "siw_base_qp" is getting freed in siw_destroy_qp(),
> >but
> >  if cm_close_handler() holds the last reference, then siw_free_qp(),
> >  via cm_close_handler(), tries to get already freed "siw_base_qp"
> >from
> >  "ib_qp".
> >   Hence, "siw_base_qp" should be freed at the end of siw_free_qp().
> >
>
> Regarding the siw driver, I am fine with that proposed
> change. Delaying freeing the base_qp is OK. In fact,
> I'd expect the drivers soon are passing that responsibility
> to the rdma core anyway -- like for CQ/SRQ/PD/CTX objects,
> which are already allocated and freed up there.
>
> The iwcm changes look OK to me as well.
>

Hey Krishna,  Since the iwcm struct/state is still correctly being
manipulated under the lock, then I think it this patch correct.  Test
the heck out of it. :)

Steve.