| Message ID | 20250215054431.55747-2-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: idxd: fix memory leak in error handling path | expand |
> Memory allocated for wqs is not freed if an error occurs during > idxd_setup_wqs(). To fix it, free the allocated memory in the reverse > order of allocation before exiting the function in case of an error. > > Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") … Will a “stable tag” become relevant also for this patch series? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3 > +++ b/drivers/dma/idxd/init.c > @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd) … > @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev)); > if (!wq->wqcfg) { > put_device(conf_dev); > + kfree(wq); > rc = -ENOMEM; > goto err; > } … I got the impression that more common exception handling code could be moved to additional jump targets at the end of such function implementations. Will further adjustment opportunities be taken into account for the affected resource management? Regards, Markus
…
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
…
Would other commits be more appropriate for the desired reference
(according to the affected function implementation)?
Examples:
2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap")
2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev")
2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime")
Regards,
Markus
在 2025/2/15 19:00, Markus Elfring 写道: >> Memory allocated for wqs is not freed if an error occurs during >> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse >> order of allocation before exiting the function in case of an error. >> >> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") > … > > Will a “stable tag” become relevant also for this patch series? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3 > I don't know if this is a real serious issue for stable kernel. But I would like to add stable tag if the mantainer @Vinicius and @Dave ask. > >> +++ b/drivers/dma/idxd/init.c >> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > … >> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev)); >> if (!wq->wqcfg) { >> put_device(conf_dev); >> + kfree(wq); >> rc = -ENOMEM; >> goto err; >> } > … > > I got the impression that more common exception handling code could be moved > to additional jump targets at the end of such function implementations. > Will further adjustment opportunities be taken into account for > the affected resource management? Sure, I will move them to the jump targets. > > Regards, > Markus Thanks for valuable comments. Shuai
在 2025/2/15 21:34, Markus Elfring 写道: > … >> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") > … > > Would other commits be more appropriate for the desired reference > (according to the affected function implementation)? > > Examples: > 2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap") > 2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev") > 2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime") > Ok, I will add the fixes. > > Regards, > Markus Thanks.
>> Will a “stable tag” become relevant also for this patch series? >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3 > > I don't know if this is a real serious issue for stable kernel. How many resource leaks would you like to avoid with your contributions? Regards, Markus
在 2025/2/16 17:34, Markus Elfring 写道: >>> Will a “stable tag” become relevant also for this patch series? >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3 >> >> I don't know if this is a real serious issue for stable kernel. > How many resource leaks would you like to avoid with your contributions? For this patch: - wqs = 8 * 8 bytes = 64 bytes - wq = 8 * 2400 bytes = 19200 bytes - opcap_bmap = 256 bit / 8 = 32 bytes Around 18 KB. > > Regards, > Markus Thanks. Shuai
Hi, Shuai, On 2/14/25 21:44, Shuai Xue wrote: > Memory allocated for wqs is not freed if an error occurs during > idxd_setup_wqs(). To fix it, free the allocated memory in the reverse > order of allocation before exiting the function in case of an error. > > Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") > Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/dma/idxd/init.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c > index b946f78f85e1..b85736fd25bd 100644 > --- a/drivers/dma/idxd/init.c > +++ b/drivers/dma/idxd/init.c > @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > > idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev)); > if (!idxd->wq_enable_map) { > - kfree(idxd->wqs); > - return -ENOMEM; > + rc = -ENOMEM; > + goto err_bitmap; > } > > for (i = 0; i < idxd->max_wqs; i++) { > @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id); > if (rc < 0) { > put_device(conf_dev); > + kfree(wq); > goto err; > } > > @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev)); > if (!wq->wqcfg) { > put_device(conf_dev); > + kfree(wq); > rc = -ENOMEM; > goto err; > } > @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > if (idxd->hw.wq_cap.op_config) { > wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL); > if (!wq->opcap_bmap) { > + kfree(wq->wqcfg); > put_device(conf_dev); > + kfree(wq); > rc = -ENOMEM; > goto err; > } > @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd) > return 0; > > err: > - while (--i >= 0) { > + while (i-- > 0) { Why changed to "i-- > 0" here? Before coming to here, the mem areas allocated for wqs[i] are freed already and there is not need to free them again here, right? And if i>1, mem areas for wqs[0] won't be freed and will leak, right? > wq = idxd->wqs[i]; > + if (idxd->hw.wq_cap.op_config) > + bitmap_free(wq->opcap_bmap); > + kfree(wq->wqcfg); > conf_dev = wq_confdev(wq); > put_device(conf_dev); > + kfree(wq); > + > } > + bitmap_free(idxd->wq_enable_map); > + > +err_bitmap: > + kfree(idxd->wqs); > + > return rc; > } > Thanks. -Fenghua
在 2025/2/19 00:32, Fenghua Yu 写道: > Hi, Shuai, > > On 2/14/25 21:44, Shuai Xue wrote: >> Memory allocated for wqs is not freed if an error occurs during >> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse >> order of allocation before exiting the function in case of an error. >> >> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") >> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> >> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/dma/idxd/init.c | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c >> index b946f78f85e1..b85736fd25bd 100644 >> --- a/drivers/dma/idxd/init.c >> +++ b/drivers/dma/idxd/init.c >> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev)); >> if (!idxd->wq_enable_map) { >> - kfree(idxd->wqs); >> - return -ENOMEM; >> + rc = -ENOMEM; >> + goto err_bitmap; >> } >> for (i = 0; i < idxd->max_wqs; i++) { >> @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id); >> if (rc < 0) { >> put_device(conf_dev); >> + kfree(wq); >> goto err; >> } >> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev)); >> if (!wq->wqcfg) { >> put_device(conf_dev); >> + kfree(wq); >> rc = -ENOMEM; >> goto err; >> } >> @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> if (idxd->hw.wq_cap.op_config) { >> wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL); >> if (!wq->opcap_bmap) { >> + kfree(wq->wqcfg); >> put_device(conf_dev); >> + kfree(wq); >> rc = -ENOMEM; >> goto err; >> } >> @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd) >> return 0; >> err: >> - while (--i >= 0) { >> + while (i-- > 0) { > > Why changed to "i-- > 0" here? Before coming to here, the mem areas allocated for wqs[i] are freed already and there is not need to free them again here, right? Yes. > And if i>1, mem areas for wqs[0] won't be freed and will leak, right? No, the two ways of writing are equivalent. #include <stdio.h> int main() { int i = 1; while (i-- > 0) printf("freeing i %d\n", i); return 0; } // console output // freeing i 0 I will drop this line to avoid confusion. Thanks. Shuai
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c index b946f78f85e1..b85736fd25bd 100644 --- a/drivers/dma/idxd/init.c +++ b/drivers/dma/idxd/init.c @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd) idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev)); if (!idxd->wq_enable_map) { - kfree(idxd->wqs); - return -ENOMEM; + rc = -ENOMEM; + goto err_bitmap; } for (i = 0; i < idxd->max_wqs; i++) { @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id); if (rc < 0) { put_device(conf_dev); + kfree(wq); goto err; } @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd) wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev)); if (!wq->wqcfg) { put_device(conf_dev); + kfree(wq); rc = -ENOMEM; goto err; } @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd) if (idxd->hw.wq_cap.op_config) { wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL); if (!wq->opcap_bmap) { + kfree(wq->wqcfg); put_device(conf_dev); + kfree(wq); rc = -ENOMEM; goto err; } @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd) return 0; err: - while (--i >= 0) { + while (i-- > 0) { wq = idxd->wqs[i]; + if (idxd->hw.wq_cap.op_config) + bitmap_free(wq->opcap_bmap); + kfree(wq->wqcfg); conf_dev = wq_confdev(wq); put_device(conf_dev); + kfree(wq); + } + bitmap_free(idxd->wq_enable_map); + +err_bitmap: + kfree(idxd->wqs); + return rc; }
Memory allocated for wqs is not freed if an error occurs during idxd_setup_wqs(). To fix it, free the allocated memory in the reverse order of allocation before exiting the function in case of an error. Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input") Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> --- drivers/dma/idxd/init.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)