This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
alllocated in nfp_resource_try_acquire(). However, when
msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
into err_fails path where res is freed, but res->mutex is not.
Fix this by changing call to free to nfp_resource_release().
Signed-off-by: Keita Suzuki <[email protected]>
---
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
index ce7492a6a98f..95e7bdc95652 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
@@ -200,7 +200,7 @@ nfp_resource_acquire(struct nfp_cpp *cpp, const char *name)
err_free:
nfp_cpp_mutex_free(dev_mutex);
- kfree(res);
+ nfp_resource_relase(res);
return ERR_PTR(err);
}
--
2.17.1
From: Keita Suzuki <[email protected]>
Date: Thu, 9 Apr 2020 15:02:07 +0000
> This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
> alllocated in nfp_resource_try_acquire(). However, when
> msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
> into err_fails path where res is freed, but res->mutex is not.
>
> Fix this by changing call to free to nfp_resource_release().
>
> Signed-off-by: Keita Suzuki <[email protected]>
Did you test compile this?
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c: In function ?nfp_resource_acquire?:
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c:203:2: error: implicit declaration of function ?nfp_resource_relase?; did you mean ?nfp_resource_release?? [-Werror=implicit-function-declaration]
nfp_resource_relase(res);
^~~~~~~~~~~~~~~~~~~
nfp_resource_release
And this makes me feel like the test was not runtime tested either.
Hi,
So sorry about this. It seems like I accidentally touched the patch
file after generating / testing the patch.
I will resend the new patch immediately.
I have tested the patch using kmemleak.
Thanks.
2020年4月10日(金) 2:18 David Miller <[email protected]>:
>
> From: Keita Suzuki <[email protected]>
> Date: Thu, 9 Apr 2020 15:02:07 +0000
>
> > This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
> > alllocated in nfp_resource_try_acquire(). However, when
> > msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
> > into err_fails path where res is freed, but res->mutex is not.
> >
> > Fix this by changing call to free to nfp_resource_release().
> >
> > Signed-off-by: Keita Suzuki <[email protected]>
>
> Did you test compile this?
>
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c: In function ‘nfp_resource_acquire’:
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c:203:2: error: implicit declaration of function ‘nfp_resource_relase’; did you mean ‘nfp_resource_release’? [-Werror=implicit-function-declaration]
> nfp_resource_relase(res);
> ^~~~~~~~~~~~~~~~~~~
> nfp_resource_release
>
> And this makes me feel like the test was not runtime tested either.
This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
alllocated in nfp_resource_try_acquire(). However, when
msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
into err_fails path where res is freed, but res->mutex is not.
Fix this by changing call to free to nfp_resource_release().
Signed-off-by: Keita Suzuki <[email protected]>
---
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
index ce7492a6a98f..95e7bdc95652 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
@@ -200,7 +200,7 @@ nfp_resource_acquire(struct nfp_cpp *cpp, const char *name)
err_free:
nfp_cpp_mutex_free(dev_mutex);
- kfree(res);
+ nfp_resource_release(res);
return ERR_PTR(err);
}
--
2.17.1
On Thu, 9 Apr 2020 17:41:11 +0000 Keita Suzuki wrote:
> This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
> alllocated in nfp_resource_try_acquire(). However, when
> msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
> into err_fails path where res is freed, but res->mutex is not.
>
> Fix this by changing call to free to nfp_resource_release().
I don't see a leak here. Maybe you could rephrase the description to
make things clearer?
AFAICS nfp_resource_try_acquire() calls nfp_cpp_mutex_free(res->mutex)
if it's about to return an error. We can only hit msleep or time check
if it returned an error.
Hi,
Thanks for reviewing.
I'll check back the runtime log and see what I can do.
Thanks.
2020年4月10日(金) 4:32 Jakub Kicinski <[email protected]>:
>
> On Thu, 9 Apr 2020 17:41:11 +0000 Keita Suzuki wrote:
> > This patch fixes a memory leak in nfp_resource_acquire(). res->mutex is
> > alllocated in nfp_resource_try_acquire(). However, when
> > msleep_interruptible() or time_is_before_eq_jiffies() fails, it falls
> > into err_fails path where res is freed, but res->mutex is not.
> >
> > Fix this by changing call to free to nfp_resource_release().
>
> I don't see a leak here. Maybe you could rephrase the description to
> make things clearer?
>
> AFAICS nfp_resource_try_acquire() calls nfp_cpp_mutex_free(res->mutex)
> if it's about to return an error. We can only hit msleep or time check
> if it returned an error.
Hi Keita,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.7-rc1 next-20200416]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Keita-Suzuki/nfp-Fix-memory-leak-in-nfp_resource_acquire/20200410-032846
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5d30bcacd91af6874481129797af364a53cd9b46
config: x86_64-rhel-7.6 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c: In function 'nfp_resource_acquire':
>> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c:203:2: error: implicit declaration of function 'nfp_resource_relase'; did you mean 'nfp_resource_release'? [-Werror=implicit-function-declaration]
nfp_resource_relase(res);
^~~~~~~~~~~~~~~~~~~
nfp_resource_release
cc1: some warnings being treated as errors
vim +203 drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.c
139
140 /**
141 * nfp_resource_acquire() - Acquire a resource handle
142 * @cpp: NFP CPP handle
143 * @name: Name of the resource
144 *
145 * NOTE: This function locks the acquired resource
146 *
147 * Return: NFP Resource handle, or ERR_PTR()
148 */
149 struct nfp_resource *
150 nfp_resource_acquire(struct nfp_cpp *cpp, const char *name)
151 {
152 unsigned long warn_at = jiffies + NFP_MUTEX_WAIT_FIRST_WARN * HZ;
153 unsigned long err_at = jiffies + NFP_MUTEX_WAIT_ERROR * HZ;
154 struct nfp_cpp_mutex *dev_mutex;
155 struct nfp_resource *res;
156 int err;
157
158 res = kzalloc(sizeof(*res), GFP_KERNEL);
159 if (!res)
160 return ERR_PTR(-ENOMEM);
161
162 strncpy(res->name, name, NFP_RESOURCE_ENTRY_NAME_SZ);
163
164 dev_mutex = nfp_cpp_mutex_alloc(cpp, NFP_RESOURCE_TBL_TARGET,
165 NFP_RESOURCE_TBL_BASE,
166 NFP_RESOURCE_TBL_KEY);
167 if (!dev_mutex) {
168 kfree(res);
169 return ERR_PTR(-ENOMEM);
170 }
171
172 for (;;) {
173 err = nfp_resource_try_acquire(cpp, res, dev_mutex);
174 if (!err)
175 break;
176 if (err != -EBUSY)
177 goto err_free;
178
179 err = msleep_interruptible(1);
180 if (err != 0) {
181 err = -ERESTARTSYS;
182 goto err_free;
183 }
184
185 if (time_is_before_eq_jiffies(warn_at)) {
186 warn_at = jiffies + NFP_MUTEX_WAIT_NEXT_WARN * HZ;
187 nfp_warn(cpp, "Warning: waiting for NFP resource %s\n",
188 name);
189 }
190 if (time_is_before_eq_jiffies(err_at)) {
191 nfp_err(cpp, "Error: resource %s timed out\n", name);
192 err = -EBUSY;
193 goto err_free;
194 }
195 }
196
197 nfp_cpp_mutex_free(dev_mutex);
198
199 return res;
200
201 err_free:
202 nfp_cpp_mutex_free(dev_mutex);
> 203 nfp_resource_relase(res);
204 return ERR_PTR(err);
205 }
206
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]