[PATCH] kernel/module: Fix mem leak in module_add_modinfo_attrs

[PATCH] kernel/module: Fix mem leak in module_add_modinfo_attrs

[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>
---
 kernel/module.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 0b9aa8a..7da73c4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1714,15 +1714,29 @@ 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:
+	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--) {
+		if (!attr->attr.name)
+			break;
+		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
+		if (attr->free)
+			attr->free(mod);
+	}
+	kfree(mod->modinfo_attrs);
 	return error;
 }
 
-- 
1.8.3.1

Re: [PATCH] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

Friendly ping...

On 2019/5/16 0:12, 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>
> ---
>  kernel/module.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 0b9aa8a..7da73c4 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1714,15 +1714,29 @@ 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:
> +	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--) {
> +		if (!attr->attr.name)
> +			break;
> +		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
> +		if (attr->free)
> +			attr->free(mod);
> +	}
> +	kfree(mod->modinfo_attrs);
>  	return error;
>  }
>  
> 

Re: [PATCH] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Jessica Yu]

+++ YueHaibing [16/05/19 00:12 +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>
>---
> kernel/module.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 0b9aa8a..7da73c4 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1714,15 +1714,29 @@ 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:
>+	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--) {

I think we need to start at --i.  If sysfs_create_file() returned
an error at index i, we call sysfs_remove_file() starting from the
previously successful call to sysfs_create_file(), i.e. at i - 1.

>+		if (!attr->attr.name)
>+			break;
>+		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
>+		if (attr->free)
>+			attr->free(mod);
>+	}
>+	kfree(mod->modinfo_attrs);
> 	return error;
> }
>
>-- 
>1.8.3.1
>
>

Re: [PATCH] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

On 2019/5/30 19:45, Jessica Yu wrote:
> +++ YueHaibing [16/05/19 00:12 +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>
>> ---
>> kernel/module.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 0b9aa8a..7da73c4 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1714,15 +1714,29 @@ 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:
>> +    for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--) {
> 
> I think we need to start at --i.  If sysfs_create_file() returned
> an error at index i, we call sysfs_remove_file() starting from the
> previously successful call to sysfs_create_file(), i.e. at i - 1.

Indeed, you are right.

will fix it in v2, thanks!

> 
>> +        if (!attr->attr.name)
>> +            break;
>> +        sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
>> +        if (attr->free)
>> +            attr->free(mod);
>> +    }
>> +    kfree(mod->modinfo_attrs);
>>     return error;
>> }
>>
>> -- 
>> 1.8.3.1
>>
>>
> 
> .
> 

[PATCH v2] kernel/module: Fix mem leak in module_add_modinfo_attrs

[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>
---
v2: free from '--i' instead of 'i--'  
---
 kernel/module.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b..78e21a7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1723,15 +1723,29 @@ 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:
+	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; --i) {
+		if (!attr->attr.name)
+			break;
+		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
+		if (attr->free)
+			attr->free(mod);
+	}
+	kfree(mod->modinfo_attrs);
 	return error;
 }
 
-- 
2.7.4

Re: [PATCH v2] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Jessica Yu]

+++ YueHaibing [30/05/19 21:43 +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>
>---
>v2: free from '--i' instead of 'i--'
>---
> kernel/module.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 6e6712b..78e21a7 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1723,15 +1723,29 @@ 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:
>+	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; --i) {

The increment step is executed after the body of the loop, so this is
still starting at i instead of i - 1. I think we need:

	for (--i; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--)

Thanks,

Jessica

>+		if (!attr->attr.name)
>+			break;
>+		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
>+		if (attr->free)
>+			attr->free(mod);
>+	}
>+	kfree(mod->modinfo_attrs);
> 	return error;
> }
>
>-- 
>2.7.4
>
>

Re: [PATCH v2] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Miroslav Benes]

On Thu, 30 May 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>
> ---
> v2: free from '--i' instead of 'i--'  
> ---
>  kernel/module.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 6e6712b..78e21a7 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1723,15 +1723,29 @@ 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:
> +	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; --i) {
> +		if (!attr->attr.name)
> +			break;
> +		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
> +		if (attr->free)
> +			attr->free(mod);
> +	}
> +	kfree(mod->modinfo_attrs);
>  	return error;
>  }

Hi,

would not be better to reuse the existing code in 
module_remove_modinfo_attrs() instead of duplication? You could add a new 
parameter "limit" or something and call the function here. I suppose the 
order does not matter and if it does you could rename it "start" and go 
backwards like in your patch.

Btw, looking more into it, it would also be possible to let 
mod_sysfs_setup() go to out_unreg_modinfo_attrs in case of an error from 
module_add_modinfo_attrs() (and then clean the error handling in 
mod_sysfs_setup() a bit). module_remove_modinfo_attrs() almost does the 
right thing, because it checks attr->attr.name. The only problem is the
last failing attribute, because it would have attr.name set, but its 
sysfs_create_file() failed. So calling sysfs_remove_file() and the rest 
would not be correct in that case.

Miroslav

Re: [PATCH v2] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

On 2019/6/3 18:47, Jessica Yu wrote:
> +++ YueHaibing [30/05/19 21:43 +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>
>> ---
>> v2: free from '--i' instead of 'i--'
>> ---
>> kernel/module.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 6e6712b..78e21a7 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1723,15 +1723,29 @@ 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:
>> +    for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; --i) {
> 
> The increment step is executed after the body of the loop, so this is
> still starting at i instead of i - 1. I think we need:
> 
>     for (--i; (attr = &mod->modinfo_attrs[i]) && i >= 0; i--)

oops, my bad, will fix it.

> 
> Thanks,
> 
> Jessica
> 
>> +        if (!attr->attr.name)
>> +            break;
>> +        sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
>> +        if (attr->free)
>> +            attr->free(mod);
>> +    }
>> +    kfree(mod->modinfo_attrs);
>>     return error;
>> }
>>
>> -- 
>> 2.7.4
>>
>>
> 
> .
> 

Re: [PATCH v2] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

On 2019/6/3 20:11, Miroslav Benes wrote:
> On Thu, 30 May 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>
>> ---
>> v2: free from '--i' instead of 'i--'  
>> ---
>>  kernel/module.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 6e6712b..78e21a7 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1723,15 +1723,29 @@ 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:
>> +	for (; (attr = &mod->modinfo_attrs[i]) && i >= 0; --i) {
>> +		if (!attr->attr.name)
>> +			break;
>> +		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
>> +		if (attr->free)
>> +			attr->free(mod);
>> +	}
>> +	kfree(mod->modinfo_attrs);
>>  	return error;
>>  }
> 
> Hi,
> 
> would not be better to reuse the existing code in 
> module_remove_modinfo_attrs() instead of duplication? You could add a new 
> parameter "limit" or something and call the function here. I suppose the 
> order does not matter and if it does you could rename it "start" and go 
> backwards like in your patch.

This make sense, try do it in v3, thanks!

> 
> Btw, looking more into it, it would also be possible to let 
> mod_sysfs_setup() go to out_unreg_modinfo_attrs in case of an error from 
> module_add_modinfo_attrs() (and then clean the error handling in 
> mod_sysfs_setup() a bit). module_remove_modinfo_attrs() almost does the 
> right thing, because it checks attr->attr.name. The only problem is the
> last failing attribute, because it would have attr.name set, but its 
> sysfs_create_file() failed. So calling sysfs_remove_file() and the rest 
> would not be correct in that case.
> 
> Miroslav
> 
> .
> 

[PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

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

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Miroslav Benes]

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

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

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
> 
> .
> 

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Miroslav Benes]

> >> -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

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Jessica Yu]

+++ 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

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Jessica Yu]

+++ 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
>
>

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Yuehaibing]

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
>>
>>
> 
> .
> 

[PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs

[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

Re: [PATCH v3] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Greg KH]

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

Re: [PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Miroslav Benes]

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

Re: [PATCH v4] kernel/module: Fix mem leak in module_add_modinfo_attrs

[Jessica Yu]

+++ 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