> On Nov 20, 2019, at 3:58 AM, Borislav Petkov wrote: > > On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: >> Right now several architectures allow their set_memory_*() family of >> functions to fail, but callers may not be checking the return values. We >> need to fix the callers and add the __must_check attribute. > > Please formulate commit messages in passive tone. "we" is ambiguous. > > From Documentation/process/submitting-patches.rst: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." > > Also, you could add a high-level summary of the failure case from: > > https://lore.kernel.org/netdev/20180628213459.28631-4-daniel@iogearbox.net/ > > as a more real-life, convincing justification for this. Thanks for pointing it out. I will fix them in v2. >> They also may not provide any level of atomicity, in the sense that >> the memory protections may be left incomplete on failure. >> This issue likely has a few >> steps on effects architectures[1]: >> 1)Have all callers of set_memory_*() helpers check the return value. >> 2)Add __much_check to all set_memory_*() helpers so that new uses do not > > __must_check > >> ignore the return value. >> 3)Add atomicity to the calls so that the memory protections aren't left in >> a partial state. >> >> Ideally, the failure of set_memory_*() should be passed up the call stack, >> and callers should examine the failure and deal with it. But currently, >> some callers just have void return type. >> >> We need to fix the callers to handle the return all the way to the top of >> stack, and it will require a large series of patches to finish all the three >> steps mentioned above. I start with kernel/module, and will move onto other >> subsystems. I am not entirely sure about the failure modes for each caller. >> So I would like to get some comments before I move forward. This single >> patch is just for fixing the return value of set_memory_*() function in >> kernel/module, and also the related callers. Any feedback would be greatly >> appreciated. >> >> [1]:https://github.com/KSPP/linux/issues/7 >> >> Signed-off-by: Tianlin Li >> --- >> arch/arm/kernel/ftrace.c | 8 +- >> arch/arm64/kernel/ftrace.c | 6 +- >> arch/nds32/kernel/ftrace.c | 6 +- >> arch/x86/kernel/ftrace.c | 13 ++- >> include/linux/module.h | 16 ++-- >> kernel/livepatch/core.c | 15 +++- >> kernel/module.c | 170 +++++++++++++++++++++++++++---------- >> kernel/trace/ftrace.c | 15 +++- >> 8 files changed, 175 insertions(+), 74 deletions(-) > > Yeah, general idea makes sense but you'd need to redo your patch ontop > of linux-next because there are some changes in flight in ftrace-land at > least and your patch won't apply anymore after next week, when the merge > window opens. > > Also, you should use checkpatch before sending a patch as sometimes it makes > sense what it complains about: > > WARNING: Missing a blank line after declarations > #79: FILE: arch/arm/kernel/ftrace.c:68: > + int ret; > + ret = set_all_modules_text_ro(); > > WARNING: Missing a blank line after declarations > #150: FILE: arch/x86/kernel/ftrace.c:61: > + int ret; > + ret = set_all_modules_text_ro(); > > WARNING: trailing semicolon indicates no statements, indent implies otherwise > #203: FILE: kernel/livepatch/core.c:731: > + if (module_enable_ro(patch->mod, true)); > + pr_err("module_enable_ro failed.\n"); > > ERROR: trailing statements should be on next line > #203: FILE: kernel/livepatch/core.c:731: > + if (module_enable_ro(patch->mod, true)); > > WARNING: Missing a blank line after declarations > #451: FILE: kernel/module.c:2091: > + int ret; > + ret = frob_text(&mod->core_layout, set_memory_x); > > WARNING: Missing a blank line after declarations > #511: FILE: kernel/trace/ftrace.c:5819: > + int ret = ftrace_arch_code_modify_prepare(); > + if (ret) { > > WARNING: Missing a blank line after declarations > #527: FILE: kernel/trace/ftrace.c:5864: > + int ret = ftrace_arch_code_modify_post_process(); > + FTRACE_WARN_ON(ret); > >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index c4ce08f43bd6..39bfc0685854 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct klp_patch *patch, >> >> mutex_lock(&text_mutex); >> >> - module_disable_ro(patch->mod); >> + ret = module_disable_ro(patch->mod); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> ret = klp_write_object_relocations(patch->mod, obj); >> if (ret) { >> - module_enable_ro(patch->mod, true); >> + if (module_enable_ro(patch->mod, true)); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > and if you look at its output above closely, it might even help you > catch the bug you've added. > > [ Don't worry, happens to the best of us. :-) ] > > Also, what would help review is if you split your patch: > > patch 1: Change functions to return a retval > patch 2-n: Change call sites to handle retval properly ok. I will redo the patch on top of linux-next. I will use checkpatch and split the patch properly in v2. Thanks for all valuable comments. Really appreciate it. > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette