On 4/23/19 5:24 PM, Jason Gunthorpe wrote:
> 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...
>
OK, What about -stable upstream versions?
> for-rc has the dealloc_driver flow, so you should use it instead of
> duplicated code like this.
>
Do you mean modify all the iWarp providers to implement dealloc_driver()
callback which will free the iwcm pointer?
>>>> 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?
>
Do you mean to pass the ifname as a parameter to ib_register_device() and modify
the ib_device_ops to include the following callbacks from iw_cm_verbs?
If that is the case then we can add the "ifname" & "driver_flags" to the
ib_device struct and remove the iw_cm_verbs struct, which will mean that no need
to implement the dealloc_driver() callback, because the iwcm pointer isn't
allocated.
struct iw_cm_verbs {
void (*add_ref)(struct ib_qp *qp);
void (*rem_ref)(struct ib_qp *qp);
struct ib_qp * (*get_qp)(struct ib_device *device,
int qpn);
int (*connect)(struct iw_cm_id *cm_id,
struct iw_cm_conn_param *conn_param);
int (*accept)(struct iw_cm_id *cm_id,
struct iw_cm_conn_param *conn_param);
int (*reject)(struct iw_cm_id *cm_id,
const void *pdata, u8 pdata_len);
int (*create_listen)(struct iw_cm_id *cm_id,
int backlog);
int (*destroy_listen)(struct iw_cm_id *cm_id);
char ifname[IFNAMSIZ];
enum iw_flags driver_flags;
};
Thanks,
Kamal
