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