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 <[email protected]>
>> ---
>> 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
>
> .
>
