On Nov 20, 2019, at 3:58 AM, Borislav Petkov <bp@alien8.de> 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.

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 <tli@digitalocean.com>
---
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

Thx.

-- 
Regards/Gruss,
   Boris.

https://people.kernel.org/tglx/notes-about-netiquette