Re: [RFC PATCH] kernel/module: have the callers of set_memory_*() check the return value

[Borislav Petkov <bp@alien8.de>]

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