| 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 |
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
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
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
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 >
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
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
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
>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
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
>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 --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);
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(-)