| Message ID | 1572072995-11277-2-git-send-email-liweihang@hisilicon.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-rc,1/2] RDMA/hns: Fix memory leaks about mr | expand |
On Sat, Oct 26, 2019 at 02:56:34PM +0800, Weihang Li wrote: > From: Lijun Ou <oulijun@huawei.com> > > In hns_roce_v1_dereg_mr(), 'mr_work' is not freed in some cases, for > example, try_wait_for_completion() runs fail, which will cause memory > leaks. > > Fixes: bfcc681bd09d ("IB/hns: Fix the bug when free mr") > Signed-off-by: Lijun Ou <oulijun@huawei.com> > Signed-off-by: Weihang Li <liweihang@hisilicon.com> > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c > index 5f74bf5..88c1cd9 100644 > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c > @@ -1094,7 +1094,6 @@ static void hns_roce_v1_mr_free_work_fn(struct work_struct *work) > free_work: > if (mr_work->comp_flag) > complete(mr_work->comp); > - kfree(mr_work); > } > > static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev, > @@ -1137,18 +1136,21 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev, > > while (end > 0) { > if (try_wait_for_completion(&comp)) > - goto free_mr; > + goto err; > msleep(HNS_ROCE_V1_FREE_MR_WAIT_VALUE); > end -= HNS_ROCE_V1_FREE_MR_WAIT_VALUE; > } > > mr_work->comp_flag = 0; > if (try_wait_for_completion(&comp)) > - goto free_mr; > + goto err; > > dev_warn(dev, "Free mr work 0x%x over 50s and failed!\n", mr->key); > ret = -ETIMEDOUT; > > +err: > + kfree(mr_work); This whole thing makes absolutely no sense. Why is work being pushed onto a WQ and then the same routine turns around and does wait_for_completion?? Further, trying to make this 'non blocking' by sleep spinning on 'try_wait_for_completion' is utterly insane. Then going on to uncondionally free memory that we don't even know if the work is finished with or not is just wrong. Doug, you took this spin loop stuff, you get to review the fixes for it! :) Jason
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c index 5f74bf5..88c1cd9 100644 --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c @@ -1094,7 +1094,6 @@ static void hns_roce_v1_mr_free_work_fn(struct work_struct *work) free_work: if (mr_work->comp_flag) complete(mr_work->comp); - kfree(mr_work); } static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev, @@ -1137,18 +1136,21 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev, while (end > 0) { if (try_wait_for_completion(&comp)) - goto free_mr; + goto err; msleep(HNS_ROCE_V1_FREE_MR_WAIT_VALUE); end -= HNS_ROCE_V1_FREE_MR_WAIT_VALUE; } mr_work->comp_flag = 0; if (try_wait_for_completion(&comp)) - goto free_mr; + goto err; dev_warn(dev, "Free mr work 0x%x over 50s and failed!\n", mr->key); ret = -ETIMEDOUT; +err: + kfree(mr_work); + free_mr: dev_dbg(dev, "Free mr 0x%x use 0x%x us.\n", mr->key, jiffies_to_usecs(jiffies) - jiffies_to_usecs(start));