On Tue, Apr 23, 2019 at 04:51:12PM +0300, Kamal Heib wrote:
>
>
> On 4/23/19 3:31 PM, Jason Gunthorpe wrote:
> > On Tue, Apr 23, 2019 at 09:24:07AM +0300, Kamal Heib wrote:
> >>
> >>
> >> On 4/22/19 6:40 PM, Jason Gunthorpe wrote:
> >>> On Mon, Apr 22, 2019 at 05:00:19PM +0300, Kamal Heib wrote:
> >>>> Make sure to release the iwcm pointer that is allocated in
> >>>> qedr_iw_register_device() but never released.
> >>>>
> >>>> Fixes: e6a38c54faf3 ("RDMA/qedr: Add support for registering an iWARP device")
> >>>> Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
> >>>> drivers/infiniband/hw/qedr/main.c | 10 +++++++++-
> >>>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
> >>>> index 996d9ecd93e0..567f55178379 100644
> >>>> +++ b/drivers/infiniband/hw/qedr/main.c
> >>>> @@ -293,7 +293,13 @@ static int qedr_register_device(struct qedr_dev *dev)
> >>>> ib_set_device_ops(&dev->ibdev, &qedr_dev_ops);
> >>>>
> >>>> dev->ibdev.driver_id = RDMA_DRIVER_QEDR;
> >>>> - return ib_register_device(&dev->ibdev, "qedr%d");
> >>>> + rc = ib_register_device(&dev->ibdev, "qedr%d");
> >>>> + if (rc) {
> >>>> + if (IS_IWARP(dev))
> >>>> + kfree(dev->ibdev.iwcm);
> >>>> + }
> >>>> +
> >>>> + return rc;
> >>>> }
> >>>>
> >>>> /* This function allocates fast-path status block memory */
> >>>> @@ -937,6 +943,8 @@ static void qedr_remove(struct qedr_dev *dev)
> >>>> * of the registered clients.
> >>>> */
> >>>> ib_unregister_device(&dev->ibdev);
> >>>> + if (IS_IWARP(dev))
> >>>> + kfree(dev->ibdev.iwcm);
> >>>
> >>> This should probably just be in a dealloc_driver callback instead of
> >>> duplicated
> >>>
> >>> Jason
> >>>
> >>
> >> I don't think so
> >
> > Why? That is is the defined place to free memory linked to the struct
> > ib_device
> >
>
> dealloc_driver() is something that introduced recently and I'm interested in
> backport this fix to RHEL versions that don't include the dealloc_driver(),
> that's why I'm targeting this fix to for-rc.
Distro constraints are not really a reason to do something sub optimal
upstream...
for-rc has the dealloc_driver flow, so you should use it instead of
duplicated code like this.
> >> I think that we need to define a new interface in the core (iwcm.c)
> >> for the IWARP devices to use when {allocate, setting ops to,
> >> release} iwcm, what do you think?
> >
> > Why?
> >
>
> 1- Avoid bugs like this.
> 2- Reduce code duplication in the providers.
> 3- Make the providers code more cleaner.
>
> I'm suggesting to introduce the following two functions and modify the IWARP
> providers to use them.
>
> int iw_cm_register_device(struct ib_device *ibdev, char *ifname,
> const struct iw_cm_ops *ops)
> {
> ibdev->iw_cm_dev = kzalloc(sizeof(*ibdev->iw_cm_dev), GFP_KERNEL);
> if (!ibdev->iw_cm_dev)
> return -ENOMEM;
>
> ibdev->iw_cm_dev->ops = ops;
> memcpy(ibdev->iw_cm_dev->ifname, ifname,
> sizeof(ibdev->iw_cm_dev->ifname));
Why not just have this as part of the normal register process and pass
the iw_cm_ops through the existing ops structure?
Jason
