Re: [PATCH v2] livepatch: core: Return EOPNOTSUPP instead of ENOSYS

[Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>]

On Wed, Feb 06, 2019 at 11:28:32AM +0100, Petr Mladek wrote:
> On Tue 2019-02-05 09:59:33, Josh Poimboeuf wrote:
> > On Tue, Feb 05, 2019 at 03:33:28AM +0900, Alice Ferrazzi wrote:
> > > From: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
> > > 
> > > As a result of an unsupported operation is better to use EOPNOTSUPP
> > > as error code.
> > > ENOSYS is only used for 'invalid syscall nr' and nothing else.
> > > 
> > > Signed-off-by: Alice Ferrazzi <alice.ferrazzi@miraclelinux.com>
> > 
> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> I have applied the patch into for-5.1/atomic-replace branch.

Sorry to jump into the discussion so late. Thinking a little more about
the check itself, previously with immediate flag an architecture can do
livepatching with limitations and without the reliable stack trace
implemented yet.

After removal of the immediate flag by
commit d0807da78e11 ("livepatch: Remove immediate feature"), every
architecture enabling livepatching is required to have implemented
reliable stack trace.  Is it a better idea to make
HAVE_RELIABLE_STACKTRACE a config dependency, which will disable
livepatching support for architectures without reliable stack trace
function during kernel build?

The idea is to remove klp_have_reliable_stack() by moving
CONFIG_HAVE_RELIABLE_STACKTRACE as a config dependency to Kconfig file
and adding the other CONFIG_STACKTRACE as a config dependency is not
required, as it's selected via CONFIG_DYNAMIC_FTRACE_WITH_REGS
dependency chain. With the patch on architecture without
HAVE_RELIABLE_STACKTRACE, the user should see:

# insmod ./livepatch-sample.ko 
insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module format

# dmesg
...
[  286.453463] livepatch_sample: module is marked as livepatch module, but livepatch support is disabled

I have done limited testing on PowerPC and to test the unsupported case,
the config dependency HAVE_RELIABLE_STACKTRACE was misspelled in Kconfig
file. If the idea sounds ok I will send a formal patch.

-------8<----------------------------

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..7848c7bbffbb 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -214,12 +214,6 @@ static inline bool klp_patch_pending(struct task_struct *task)
        return test_tsk_thread_flag(task, TIF_PATCH_PENDING);
 }
 
-static inline bool klp_have_reliable_stack(void)
-{
-       return IS_ENABLED(CONFIG_STACKTRACE) &&
-              IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
-}
-
 typedef int (*klp_shadow_ctor_t)(void *obj,
                                 void *shadow_data,
                                 void *ctor_data);
diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
index ec4565122e65..16b3692ddf9f 100644
--- a/kernel/livepatch/Kconfig
+++ b/kernel/livepatch/Kconfig
@@ -9,6 +9,7 @@ config LIVEPATCH
        depends on MODULES
        depends on SYSFS
        depends on KALLSYMS_ALL
+       depends on HAVE_RELIABLE_STACKTRACE
        depends on HAVE_LIVEPATCH
        depends on !TRIM_UNUSED_KSYMS
        help
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index fe1993399823..9a80f7574d75 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1002,12 +1002,6 @@ int klp_enable_patch(struct klp_patch *patch)
        if (!klp_initialized())
                return -ENODEV;
 
-       if (!klp_have_reliable_stack()) {
-               pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
-               return -ENOSYS;
-       }
-
-
        mutex_lock(&klp_mutex);
 
        ret = klp_init_patch_early(patch);