* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-03 14:45 ` [PATCH v3] " YueHaibing
@ 2019-06-04 10:46 ` Miroslav Benes
2019-06-04 13:54 ` Yuehaibing
2019-06-07 14:02 ` Jessica Yu
2019-06-11 13:33 ` Jessica Yu
2019-06-11 15:00 ` [PATCH v4] " YueHaibing
2 siblings, 2 replies; 20+ messages in thread
From: Miroslav Benes @ 2019-06-04 10:46 UTC (permalink / raw)
To: YueHaibing; +Cc: jeyu, gregkh, linux-kernel
On Mon, 3 Jun 2019, YueHaibing wrote:
> In module_add_modinfo_attrs if sysfs_create_file
> fails, we forget to free allocated modinfo_attrs
> and roll back the sysfs files.
>
> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> v3: reuse module_remove_modinfo_attrs
> v2: free from '--i' instead of 'i--'
> ---
> kernel/module.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
I'm afraid it is not completely correct.
> diff --git a/kernel/module.c b/kernel/module.c
> index 80c7c09..c6b8912 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
> return ret;
> }
>
> +static void module_remove_modinfo_attrs(struct module *mod, int end);
> +
> static int module_add_modinfo_attrs(struct module *mod)
> {
> struct module_attribute *attr;
> @@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
> return -ENOMEM;
>
> temp_attr = mod->modinfo_attrs;
> - for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
> + for (i = 0; (attr = modinfo_attrs[i]); i++) {
> if (!attr->test || attr->test(mod)) {
> memcpy(temp_attr, attr, sizeof(*temp_attr));
> sysfs_attr_init(&temp_attr->attr);
> error = sysfs_create_file(&mod->mkobj.kobj,
> &temp_attr->attr);
> + if (error)
> + goto error_out;
sysfs_create_file() failed, so we need to clear all previously processed
attrs and not the current one.
> ++temp_attr;
> }
> }
> +
> + return 0;
> +
> +error_out:
> + module_remove_modinfo_attrs(mod, --i);
It says "call sysfs_remove_file() on all attrs ending with --i included
(all correctly processed attrs).
> return error;
> }
>
> -static void module_remove_modinfo_attrs(struct module *mod)
> +static void module_remove_modinfo_attrs(struct module *mod, int end)
> {
> struct module_attribute *attr;
> int i;
>
> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
> + if (end >= 0 && i > end)
> + break;
If end == 0, you break the loop without calling sysfs_remove_file(), which
is a bug if you called module_remove_modinfo_attrs(mod, 0).
Calling module_remove_modinfo_attrs(mod, i); in module_add_modinfo_attrs()
under error_out label and changing the condition here to
if (end >= 0 && i >= end)
break;
should work as expected.
But let me ask another question and it might be more to Jessica. Why is
there even a call to attr->free(mod); (if it exists) in
module_remove_modinfo_attrs()? The same is in free_modinfo() (as opposed
to setup_modinfo() where attr->setup(mod) is called. Is it because
free_modinfo() is called only in load_module()'s error path, while
module_remove_modinfo_attrs() is called even in free_module() path?
kfree() checks for NULL pointer, so there is no bug, but it is certainly
not nice and it calls for cleanup. But I may be missing something.
Regards,
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-04 10:46 ` Miroslav Benes
@ 2019-06-04 13:54 ` Yuehaibing
2019-06-04 14:15 ` Miroslav Benes
2019-06-07 14:02 ` Jessica Yu
1 sibling, 1 reply; 20+ messages in thread
From: Yuehaibing @ 2019-06-04 13:54 UTC (permalink / raw)
To: Miroslav Benes; +Cc: jeyu, gregkh, linux-kernel
On 2019/6/4 18:46, Miroslav Benes wrote:
> On Mon, 3 Jun 2019, YueHaibing wrote:
>
>> In module_add_modinfo_attrs if sysfs_create_file
>> fails, we forget to free allocated modinfo_attrs
>> and roll back the sysfs files.
>>
>> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> v3: reuse module_remove_modinfo_attrs
>> v2: free from '--i' instead of 'i--'
>> ---
>> kernel/module.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> I'm afraid it is not completely correct.
>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 80c7c09..c6b8912 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
>> return ret;
>> }
>>
>> +static void module_remove_modinfo_attrs(struct module *mod, int end);
>> +
>> static int module_add_modinfo_attrs(struct module *mod)
>> {
>> struct module_attribute *attr;
>> @@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
>> return -ENOMEM;
>>
>> temp_attr = mod->modinfo_attrs;
>> - for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
>> + for (i = 0; (attr = modinfo_attrs[i]); i++) {
>> if (!attr->test || attr->test(mod)) {
>> memcpy(temp_attr, attr, sizeof(*temp_attr));
>> sysfs_attr_init(&temp_attr->attr);
>> error = sysfs_create_file(&mod->mkobj.kobj,
>> &temp_attr->attr);
>> + if (error)
>> + goto error_out;
>
> sysfs_create_file() failed, so we need to clear all previously processed
> attrs and not the current one.
>
>> ++temp_attr;
>> }
>> }
>> +
>> + return 0;
>> +
>> +error_out:
>> + module_remove_modinfo_attrs(mod, --i);
>
> It says "call sysfs_remove_file() on all attrs ending with --i included
> (all correctly processed attrs).
>
>> return error;
>> }
>>
>> -static void module_remove_modinfo_attrs(struct module *mod)
>> +static void module_remove_modinfo_attrs(struct module *mod, int end)
>> {
>> struct module_attribute *attr;
>> int i;
>>
>> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
>> + if (end >= 0 && i > end)
>> + break;
>
> If end == 0, you break the loop without calling sysfs_remove_file(), which
> is a bug if you called module_remove_modinfo_attrs(mod, 0).
If end == 0 and i == 0, if statement is false, it won't break the loop.
At other places, I use end == -1, which means clean all and keeps the old behavior
- module_remove_modinfo_attrs(mod);
+ module_remove_modinfo_attrs(mod, -1);
>
> Calling module_remove_modinfo_attrs(mod, i); in module_add_modinfo_attrs()
> under error_out label and changing the condition here to
>
> if (end >= 0 && i >= end)
> break;
>
> should work as expected.
>
> But let me ask another question and it might be more to Jessica. Why is
> there even a call to attr->free(mod); (if it exists) in
> module_remove_modinfo_attrs()? The same is in free_modinfo() (as opposed
> to setup_modinfo() where attr->setup(mod) is called. Is it because
> free_modinfo() is called only in load_module()'s error path, while
> module_remove_modinfo_attrs() is called even in free_module() path?
>
> kfree() checks for NULL pointer, so there is no bug, but it is certainly
> not nice and it calls for cleanup. But I may be missing something.
>
> Regards,
> Miroslav
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-04 13:54 ` Yuehaibing
@ 2019-06-04 14:15 ` Miroslav Benes
0 siblings, 0 replies; 20+ messages in thread
From: Miroslav Benes @ 2019-06-04 14:15 UTC (permalink / raw)
To: Yuehaibing; +Cc: jeyu, gregkh, linux-kernel
> >> -static void module_remove_modinfo_attrs(struct module *mod)
> >> +static void module_remove_modinfo_attrs(struct module *mod, int end)
> >> {
> >> struct module_attribute *attr;
> >> int i;
> >>
> >> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
> >> + if (end >= 0 && i > end)
> >> + break;
> >
> > If end == 0, you break the loop without calling sysfs_remove_file(), which
> > is a bug if you called module_remove_modinfo_attrs(mod, 0).
>
> If end == 0 and i == 0, if statement is false, it won't break the loop.
Eh, you're right of course.
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-04 10:46 ` Miroslav Benes
2019-06-04 13:54 ` Yuehaibing
@ 2019-06-07 14:02 ` Jessica Yu
1 sibling, 0 replies; 20+ messages in thread
From: Jessica Yu @ 2019-06-07 14:02 UTC (permalink / raw)
To: Miroslav Benes; +Cc: YueHaibing, gregkh, linux-kernel
+++ Miroslav Benes [04/06/19 12:46 +0200]:
>On Mon, 3 Jun 2019, YueHaibing wrote:
>
>> In module_add_modinfo_attrs if sysfs_create_file
>> fails, we forget to free allocated modinfo_attrs
>> and roll back the sysfs files.
>>
>> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> v3: reuse module_remove_modinfo_attrs
>> v2: free from '--i' instead of 'i--'
>> ---
>> kernel/module.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>I'm afraid it is not completely correct.
>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 80c7c09..c6b8912 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
>> return ret;
>> }
>>
>> +static void module_remove_modinfo_attrs(struct module *mod, int end);
>> +
>> static int module_add_modinfo_attrs(struct module *mod)
>> {
>> struct module_attribute *attr;
>> @@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
>> return -ENOMEM;
>>
>> temp_attr = mod->modinfo_attrs;
>> - for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
>> + for (i = 0; (attr = modinfo_attrs[i]); i++) {
>> if (!attr->test || attr->test(mod)) {
>> memcpy(temp_attr, attr, sizeof(*temp_attr));
>> sysfs_attr_init(&temp_attr->attr);
>> error = sysfs_create_file(&mod->mkobj.kobj,
>> &temp_attr->attr);
>> + if (error)
>> + goto error_out;
>
>sysfs_create_file() failed, so we need to clear all previously processed
>attrs and not the current one.
>
>> ++temp_attr;
>> }
>> }
>> +
>> + return 0;
>> +
>> +error_out:
>> + module_remove_modinfo_attrs(mod, --i);
>
>It says "call sysfs_remove_file() on all attrs ending with --i included
>(all correctly processed attrs).
>
>> return error;
>> }
>>
>> -static void module_remove_modinfo_attrs(struct module *mod)
>> +static void module_remove_modinfo_attrs(struct module *mod, int end)
>> {
>> struct module_attribute *attr;
>> int i;
>>
>> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
>> + if (end >= 0 && i > end)
>> + break;
>
>If end == 0, you break the loop without calling sysfs_remove_file(), which
>is a bug if you called module_remove_modinfo_attrs(mod, 0).
>
>Calling module_remove_modinfo_attrs(mod, i); in module_add_modinfo_attrs()
>under error_out label and changing the condition here to
>
>if (end >= 0 && i >= end)
> break;
>
>should work as expected.
>
>But let me ask another question and it might be more to Jessica. Why is
>there even a call to attr->free(mod); (if it exists) in
>module_remove_modinfo_attrs()? The same is in free_modinfo() (as opposed
>to setup_modinfo() where attr->setup(mod) is called. Is it because
>free_modinfo() is called only in load_module()'s error path, while
>module_remove_modinfo_attrs() is called even in free_module() path?
>
>kfree() checks for NULL pointer, so there is no bug, but it is certainly
>not nice and it calls for cleanup. But I may be missing something.
No, you are right in that it is a bit clumsy and and the sysfs error
path handling is asymmetrical. I think it could be cleaned up a bit.
IMO, I think the attr->free() calls should either be (1) removed from
module_remove_modinfo_attrs() as free_modinfo() takes care of that,
otherwise we could potentially call attr->free() twice (once in the
internal error handling of mod_sysfs_setup() and once again in the
free_modinfo: label in load_module()) or option (2) would be to merge
the attr->setup() calls into module_add_modinfo_attrs() so that it is
symmetrical to module_remove_modinfo_attrs(). I'm leaning towards
option 2 but have not carefully checked yet if moving the
attr->setup() calls into module_add_modinfo_attrs() would break
anything. In any case I will prepare some cleanup patches for this.
Thanks!
Jessica
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-03 14:45 ` [PATCH v3] " YueHaibing
2019-06-04 10:46 ` Miroslav Benes
@ 2019-06-11 13:33 ` Jessica Yu
2019-06-11 14:30 ` Yuehaibing
2019-06-11 15:00 ` [PATCH v4] " YueHaibing
2 siblings, 1 reply; 20+ messages in thread
From: Jessica Yu @ 2019-06-11 13:33 UTC (permalink / raw)
To: YueHaibing; +Cc: gregkh, mbenes, linux-kernel
+++ YueHaibing [03/06/19 22:45 +0800]:
>In module_add_modinfo_attrs if sysfs_create_file
>fails, we forget to free allocated modinfo_attrs
>and roll back the sysfs files.
>
>Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
>Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>---
>v3: reuse module_remove_modinfo_attrs
>v2: free from '--i' instead of 'i--'
>---
> kernel/module.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 80c7c09..c6b8912 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
> return ret;
> }
>
>+static void module_remove_modinfo_attrs(struct module *mod, int end);
>+
> static int module_add_modinfo_attrs(struct module *mod)
> {
> struct module_attribute *attr;
>@@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
> return -ENOMEM;
>
> temp_attr = mod->modinfo_attrs;
>- for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
>+ for (i = 0; (attr = modinfo_attrs[i]); i++) {
> if (!attr->test || attr->test(mod)) {
> memcpy(temp_attr, attr, sizeof(*temp_attr));
> sysfs_attr_init(&temp_attr->attr);
> error = sysfs_create_file(&mod->mkobj.kobj,
> &temp_attr->attr);
>+ if (error)
>+ goto error_out;
> ++temp_attr;
> }
> }
>+
>+ return 0;
>+
>+error_out:
>+ module_remove_modinfo_attrs(mod, --i);
Gah, I think there is another issue here - if sysfs_create_file()
fails on the first iteration of the loop (so i = 0), then we will go
to error_out and end up calling module_remove_modinfo_attrs(mod, -1),
which, for i = 0, will call sysfs_remove_file() since attr->attr.name
had already been set before the failed call to sysfs_create_file().
Perhaps we need to check that i > 0 before calling
module_remove_modinfo_attrs() under error_out?
> return error;
> }
>
>-static void module_remove_modinfo_attrs(struct module *mod)
>+static void module_remove_modinfo_attrs(struct module *mod, int end)
> {
> struct module_attribute *attr;
> int i;
>
> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
>+ if (end >= 0 && i > end)
>+ break;
> /* pick a field to test for end of list */
> if (!attr->attr.name)
> break;
>@@ -1816,7 +1827,7 @@ static int mod_sysfs_setup(struct module *mod,
> return 0;
>
> out_unreg_modinfo_attrs:
>- module_remove_modinfo_attrs(mod);
>+ module_remove_modinfo_attrs(mod, -1);
> out_unreg_param:
> module_param_sysfs_remove(mod);
> out_unreg_holders:
>@@ -1852,7 +1863,7 @@ static void mod_sysfs_fini(struct module *mod)
> {
> }
>
>-static void module_remove_modinfo_attrs(struct module *mod)
>+static void module_remove_modinfo_attrs(struct module *mod, int end)
> {
> }
>
>@@ -1868,7 +1879,7 @@ static void init_param_lock(struct module *mod)
> static void mod_sysfs_teardown(struct module *mod)
> {
> del_usage_links(mod);
>- module_remove_modinfo_attrs(mod);
>+ module_remove_modinfo_attrs(mod, -1);
> module_param_sysfs_remove(mod);
> kobject_put(mod->mkobj.drivers_dir);
> kobject_put(mod->holders_dir);
>--
>1.8.3.1
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-11 13:33 ` Jessica Yu
@ 2019-06-11 14:30 ` Yuehaibing
2019-06-11 15:38 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Yuehaibing @ 2019-06-11 14:30 UTC (permalink / raw)
To: Jessica Yu; +Cc: gregkh, mbenes, linux-kernel
On 2019/6/11 21:33, Jessica Yu wrote:
> +++ YueHaibing [03/06/19 22:45 +0800]:
>> In module_add_modinfo_attrs if sysfs_create_file
>> fails, we forget to free allocated modinfo_attrs
>> and roll back the sysfs files.
>>
>> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> v3: reuse module_remove_modinfo_attrs
>> v2: free from '--i' instead of 'i--'
>> ---
>> kernel/module.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 80c7c09..c6b8912 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
>> return ret;
>> }
>>
>> +static void module_remove_modinfo_attrs(struct module *mod, int end);
>> +
>> static int module_add_modinfo_attrs(struct module *mod)
>> {
>> struct module_attribute *attr;
>> @@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
>> return -ENOMEM;
>>
>> temp_attr = mod->modinfo_attrs;
>> - for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
>> + for (i = 0; (attr = modinfo_attrs[i]); i++) {
>> if (!attr->test || attr->test(mod)) {
>> memcpy(temp_attr, attr, sizeof(*temp_attr));
>> sysfs_attr_init(&temp_attr->attr);
>> error = sysfs_create_file(&mod->mkobj.kobj,
>> &temp_attr->attr);
>> + if (error)
>> + goto error_out;
>> ++temp_attr;
>> }
>> }
>> +
>> + return 0;
>> +
>> +error_out:
>> + module_remove_modinfo_attrs(mod, --i);
>
> Gah, I think there is another issue here - if sysfs_create_file()
> fails on the first iteration of the loop (so i = 0), then we will go
> to error_out and end up calling module_remove_modinfo_attrs(mod, -1),
> which, for i = 0, will call sysfs_remove_file() since attr->attr.name
> had already been set before the failed call to sysfs_create_file().
> Perhaps we need to check that i > 0 before calling
> module_remove_modinfo_attrs() under error_out?
Indeed, this should be checked, thanks!
>
>> return error;
>> }
>>
>> -static void module_remove_modinfo_attrs(struct module *mod)
>> +static void module_remove_modinfo_attrs(struct module *mod, int end)
>> {
>> struct module_attribute *attr;
>> int i;
>>
>> for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
>> + if (end >= 0 && i > end)
>> + break;
>> /* pick a field to test for end of list */
>> if (!attr->attr.name)
>> break;
>> @@ -1816,7 +1827,7 @@ static int mod_sysfs_setup(struct module *mod,
>> return 0;
>>
>> out_unreg_modinfo_attrs:
>> - module_remove_modinfo_attrs(mod);
>> + module_remove_modinfo_attrs(mod, -1);
>> out_unreg_param:
>> module_param_sysfs_remove(mod);
>> out_unreg_holders:
>> @@ -1852,7 +1863,7 @@ static void mod_sysfs_fini(struct module *mod)
>> {
>> }
>>
>> -static void module_remove_modinfo_attrs(struct module *mod)
>> +static void module_remove_modinfo_attrs(struct module *mod, int end)
>> {
>> }
>>
>> @@ -1868,7 +1879,7 @@ static void init_param_lock(struct module *mod)
>> static void mod_sysfs_teardown(struct module *mod)
>> {
>> del_usage_links(mod);
>> - module_remove_modinfo_attrs(mod);
>> + module_remove_modinfo_attrs(mod, -1);
>> module_param_sysfs_remove(mod);
>> kobject_put(mod->mkobj.drivers_dir);
>> kobject_put(mod->holders_dir);
>> --
>> 1.8.3.1
>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-11 14:30 ` Yuehaibing
@ 2019-06-11 15:38 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2019-06-11 15:38 UTC (permalink / raw)
To: Yuehaibing; +Cc: Jessica Yu, mbenes, linux-kernel
On Tue, Jun 11, 2019 at 10:30:56PM +0800, Yuehaibing wrote:
> On 2019/6/11 21:33, Jessica Yu wrote:
> > +++ YueHaibing [03/06/19 22:45 +0800]:
> >> In module_add_modinfo_attrs if sysfs_create_file
> >> fails, we forget to free allocated modinfo_attrs
> >> and roll back the sysfs files.
> >>
> >> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> >> ---
> >> v3: reuse module_remove_modinfo_attrs
> >> v2: free from '--i' instead of 'i--'
> >> ---
> >> kernel/module.c | 21 ++++++++++++++++-----
> >> 1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/module.c b/kernel/module.c
> >> index 80c7c09..c6b8912 100644
> >> --- a/kernel/module.c
> >> +++ b/kernel/module.c
> >> @@ -1697,6 +1697,8 @@ static int add_usage_links(struct module *mod)
> >> return ret;
> >> }
> >>
> >> +static void module_remove_modinfo_attrs(struct module *mod, int end);
> >> +
> >> static int module_add_modinfo_attrs(struct module *mod)
> >> {
> >> struct module_attribute *attr;
> >> @@ -1711,24 +1713,33 @@ static int module_add_modinfo_attrs(struct module *mod)
> >> return -ENOMEM;
> >>
> >> temp_attr = mod->modinfo_attrs;
> >> - for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
> >> + for (i = 0; (attr = modinfo_attrs[i]); i++) {
> >> if (!attr->test || attr->test(mod)) {
> >> memcpy(temp_attr, attr, sizeof(*temp_attr));
> >> sysfs_attr_init(&temp_attr->attr);
> >> error = sysfs_create_file(&mod->mkobj.kobj,
> >> &temp_attr->attr);
> >> + if (error)
> >> + goto error_out;
> >> ++temp_attr;
> >> }
> >> }
> >> +
> >> + return 0;
> >> +
> >> +error_out:
> >> + module_remove_modinfo_attrs(mod, --i);
> >
> > Gah, I think there is another issue here - if sysfs_create_file()
> > fails on the first iteration of the loop (so i = 0), then we will go
> > to error_out and end up calling module_remove_modinfo_attrs(mod, -1),
> > which, for i = 0, will call sysfs_remove_file() since attr->attr.name
> > had already been set before the failed call to sysfs_create_file().
> > Perhaps we need to check that i > 0 before calling
> > module_remove_modinfo_attrs() under error_out?
>
> Indeed, this should be checked, thanks!
There is a simple sysfs core function that does the whole "add a whole
group of files at once" in a simple manner. Perhaps you should be
calling that one instead?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-03 14:45 ` [PATCH v3] " YueHaibing
2019-06-04 10:46 ` Miroslav Benes
2019-06-11 13:33 ` Jessica Yu
@ 2019-06-11 15:00 ` YueHaibing
2019-06-12 11:12 ` Miroslav Benes
2019-06-14 7:54 ` Jessica Yu
2 siblings, 2 replies; 20+ messages in thread
From: YueHaibing @ 2019-06-11 15:00 UTC (permalink / raw)
To: jeyu, gregkh, mbenes; +Cc: linux-kernel, YueHaibing
In module_add_modinfo_attrs if sysfs_create_file
fails, we forget to free allocated modinfo_attrs
and roll back the sysfs files.
Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v4: call module_remove_modinfo_attrs only while i > 0
v3: reuse module_remove_modinfo_attrs
v2: free from '--i' instead of 'i--'
---
kernel/module.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index f954d72..3310a39 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1696,6 +1696,8 @@ static int add_usage_links(struct module *mod)
return ret;
}
+static void module_remove_modinfo_attrs(struct module *mod, int end);
+
static int module_add_modinfo_attrs(struct module *mod)
{
struct module_attribute *attr;
@@ -1710,24 +1712,34 @@ static int module_add_modinfo_attrs(struct module *mod)
return -ENOMEM;
temp_attr = mod->modinfo_attrs;
- for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
+ for (i = 0; (attr = modinfo_attrs[i]); i++) {
if (!attr->test || attr->test(mod)) {
memcpy(temp_attr, attr, sizeof(*temp_attr));
sysfs_attr_init(&temp_attr->attr);
error = sysfs_create_file(&mod->mkobj.kobj,
&temp_attr->attr);
+ if (error)
+ goto error_out;
++temp_attr;
}
}
+
+ return 0;
+
+error_out:
+ if (i > 0)
+ module_remove_modinfo_attrs(mod, --i);
return error;
}
-static void module_remove_modinfo_attrs(struct module *mod)
+static void module_remove_modinfo_attrs(struct module *mod, int end)
{
struct module_attribute *attr;
int i;
for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
+ if (end >= 0 && i > end)
+ break;
/* pick a field to test for end of list */
if (!attr->attr.name)
break;
@@ -1815,7 +1827,7 @@ static int mod_sysfs_setup(struct module *mod,
return 0;
out_unreg_modinfo_attrs:
- module_remove_modinfo_attrs(mod);
+ module_remove_modinfo_attrs(mod, -1);
out_unreg_param:
module_param_sysfs_remove(mod);
out_unreg_holders:
@@ -1851,7 +1863,7 @@ static void mod_sysfs_fini(struct module *mod)
{
}
-static void module_remove_modinfo_attrs(struct module *mod)
+static void module_remove_modinfo_attrs(struct module *mod, int end)
{
}
@@ -1867,7 +1879,7 @@ static void init_param_lock(struct module *mod)
static void mod_sysfs_teardown(struct module *mod)
{
del_usage_links(mod);
- module_remove_modinfo_attrs(mod);
+ module_remove_modinfo_attrs(mod, -1);
module_param_sysfs_remove(mod);
kobject_put(mod->mkobj.drivers_dir);
kobject_put(mod->holders_dir);
--
2.7.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-11 15:00 ` [PATCH v4] " YueHaibing
@ 2019-06-12 11:12 ` Miroslav Benes
2019-06-14 7:54 ` Jessica Yu
1 sibling, 0 replies; 20+ messages in thread
From: Miroslav Benes @ 2019-06-12 11:12 UTC (permalink / raw)
To: YueHaibing; +Cc: jeyu, gregkh, linux-kernel
On Tue, 11 Jun 2019, YueHaibing wrote:
> In module_add_modinfo_attrs if sysfs_create_file
> fails, we forget to free allocated modinfo_attrs
> and roll back the sysfs files.
>
> Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Miroslav
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs
2019-06-11 15:00 ` [PATCH v4] " YueHaibing
2019-06-12 11:12 ` Miroslav Benes
@ 2019-06-14 7:54 ` Jessica Yu
1 sibling, 0 replies; 20+ messages in thread
From: Jessica Yu @ 2019-06-14 7:54 UTC (permalink / raw)
To: YueHaibing; +Cc: gregkh, mbenes, linux-kernel
+++ YueHaibing [11/06/19 23:00 +0800]:
>In module_add_modinfo_attrs if sysfs_create_file
>fails, we forget to free allocated modinfo_attrs
>and roll back the sysfs files.
>
>Fixes: 03e88ae1b13d ("[PATCH] fix module sysfs files reference counting")
>Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied, thanks.
Jessica
^ permalink raw reply [flat|nested] 20+ messages in thread