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.
>> 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));
return 0;
}
EXPORT_SYMBOL(iw_cm_register_device);
void iw_cm_unregister_device(struct ib_device *ibdev)
{
kfree(ibdev->iw_cm_dev);
}
EXPORT_SYMBOL(iw_cm_unregister_device);
Thanks,
Kamal
> Jason
>
