diff mbox series

[for-rc] RDMA/qder: Fix memory leak of iwcm pointer

Message ID 20190422140019.13102-1-kamalheib1@gmail.com (mailing list archive)
State Superseded
Headers show
Series [for-rc] RDMA/qder: Fix memory leak of iwcm pointer | expand

Commit Message

Kamal Heib April 22, 2019, 2 p.m. UTC
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@gmail.com>
---
 drivers/infiniband/hw/qedr/main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe April 22, 2019, 3:40 p.m. UTC | #1
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@gmail.com>
>  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
Kamal Heib April 23, 2019, 6:24 a.m. UTC | #2
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@gmail.com>
>>  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, 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?

With regards this patch, I think that this patch needs to be sent to -stable as
a bug fix.

Thanks,
Kamal
Jason Gunthorpe April 23, 2019, 12:31 p.m. UTC | #3
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@gmail.com>
> >>  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

> 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?

Jason
Kamal Heib April 23, 2019, 1:51 p.m. UTC | #4
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@gmail.com>
>>>>  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
>
Jason Gunthorpe April 23, 2019, 2:24 p.m. UTC | #5
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@gmail.com>
> >>>>  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
Kamal Heib April 23, 2019, 4:44 p.m. UTC | #6
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@gmail.com>
>>>>>>  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
Jason Gunthorpe April 23, 2019, 4:52 p.m. UTC | #7
On Tue, Apr 23, 2019 at 07:44:51PM +0300, Kamal Heib wrote:
> >> 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?

Greg takes dependent patches generally if asked
 
> >>>> 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()

No, drivers can set it before calling register_device - not really
sure what it is for.

> and modify the ib_device_ops to include the following callbacks from
> iw_cm_verbs?

That or a pointer to a struct with them, yes.

> 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.

Yes, this makes more sense since there is really nothing in this
struct except ops function pointers which belong in the ops struct anyhow.

Jason
Shiraz Saleem April 24, 2019, 9:10 p.m. UTC | #8
>Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm pointer
>
>On Tue, Apr 23, 2019 at 07:44:51PM +0300, Kamal Heib wrote:
>> >> 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?
>
>Greg takes dependent patches generally if asked
>
>> >>>> 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()
>
>No, drivers can set it before calling register_device - not really sure what it is for.
>
>> and modify the ib_device_ops to include the following callbacks from
>> iw_cm_verbs?
>
>That or a pointer to a struct with them, yes.
>
>> 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.
>
>Yes, this makes more sense since there is really nothing in this struct except ops
>function pointers which belong in the ops struct anyhow.
>

Jason/Kamal - This was brought up during the irdma RFC review as well and we already have a patch
for this. Can send it out tomorrow.

Shiraz
Kamal Heib April 25, 2019, 7 a.m. UTC | #9
On 4/25/19 12:10 AM, Saleem, Shiraz wrote:
>> Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm pointer
>>
>> On Tue, Apr 23, 2019 at 07:44:51PM +0300, Kamal Heib wrote:
>>>>> 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?
>>
>> Greg takes dependent patches generally if asked
>>
>>>>>>> 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()
>>
>> No, drivers can set it before calling register_device - not really sure what it is for.
>>
>>> and modify the ib_device_ops to include the following callbacks from
>>> iw_cm_verbs?
>>
>> That or a pointer to a struct with them, yes.
>>
>>> 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.
>>
>> Yes, this makes more sense since there is really nothing in this struct except ops
>> function pointers which belong in the ops struct anyhow.
>>
> 
> Jason/Kamal - This was brought up during the irdma RFC review as well and we already have a patch
> for this. Can send it out tomorrow.
> 
> Shiraz
> 

Hi Shiraz,

I already prepared the patch and I'm doing some testing for it right now (before
posting it).

Thanks,
Kamal
Shiraz Saleem April 25, 2019, 1:32 p.m. UTC | #10
>Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm pointer
>
>
>
>On 4/25/19 12:10 AM, Saleem, Shiraz wrote:
>>> Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm
>>> pointer
>>>
>>> On Tue, Apr 23, 2019 at 07:44:51PM +0300, Kamal Heib wrote:
>>>>>> 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?
>>>
>>> Greg takes dependent patches generally if asked
>>>
>>>>>>>> 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()
>>>
>>> No, drivers can set it before calling register_device - not really sure what it is for.
>>>
>>>> and modify the ib_device_ops to include the following callbacks from
>>>> iw_cm_verbs?
>>>
>>> That or a pointer to a struct with them, yes.
>>>
>>>> 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.
>>>
>>> Yes, this makes more sense since there is really nothing in this
>>> struct except ops function pointers which belong in the ops struct anyhow.
>>>
>>
>> Jason/Kamal - This was brought up during the irdma RFC review as well
>> and we already have a patch for this. Can send it out tomorrow.
>>
>> Shiraz
>>
>
>Hi Shiraz,
>
>I already prepared the patch and I'm doing some testing for it right now (before posting
>it).
>

Ah ok! Go ahead and post it then. I ll back off. Thanks!
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index 996d9ecd93e0..567f55178379 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ 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);
 
 	qedr_stop_hw(dev);
 	qedr_sync_free_irqs(dev);