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

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

[Alice Ferrazzi]

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>
---
Changes v1->v2:
- Use EOPNOTSUPP instead of ENOTSUPP (Petr)

 kernel/livepatch/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index fe1993399823..075eb5c0813d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1004,7 +1004,7 @@ int klp_enable_patch(struct klp_patch *patch)
 
 	if (!klp_have_reliable_stack()) {
 		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 	}
 
 
-- 
2.19.2

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

[Petr Mladek]

On Tue 2019-02-05 03:33:28, 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: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

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

[Miroslav Benes]

On Tue, 5 Feb 2019, 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: Miroslav Benes <mbenes@suse.cz>

Miroslav

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

[Josh Poimboeuf]

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>

-- 
Josh

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

[Petr Mladek]

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.

Best Regards,
Petr

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

[Kamalesh Babulal]

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);

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

[Miroslav Benes]

Hi Kamalesh,

On Fri, 8 Feb 2019, Kamalesh Babulal wrote:

> 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?

if I am not mistaken, s390x is currently the only one which is supported 
(the redirection works) but has no reliable stacktraces (so far, it is my 
plan to take a look soon).

Theoretically, it could still work. We have the fake signal and we can 
force the remaining tasks (kthreads). It is not something to be used in 
production but it could make sense for a limited testing.
 
> 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);

On the other hand, I like this change. So we have two options, I think. 
We can apply this and wait if someone complains (because of s390x 
testing), or we can wait for the full support of s390x and then enforce 
it.

Thanks,
Miroslav

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

[Kamalesh Babulal]

Hi Miroslav,

On Fri, Feb 08, 2019 at 10:24:21AM +0100, Miroslav Benes wrote:
> Hi Kamalesh,
> 
> On Fri, 8 Feb 2019, Kamalesh Babulal wrote:
> 
> > 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.

[...]

> > 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?
> 
> if I am not mistaken, s390x is currently the only one which is supported 
> (the redirection works) but has no reliable stacktraces (so far, it is my 
> plan to take a look soon).
> 
> Theoretically, it could still work. We have the fake signal and we can 
> force the remaining tasks (kthreads). It is not something to be used in 
> production but it could make sense for a limited testing.

That was my understanding too, s390 doesn't set HAVE_RELIABLE_STACKTRACE.

(below output is right trimmed for readability)

arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_LIVEPATCH"                                                                                                                                                                               
./powerpc/Kconfig:209:  select HAVE_LIVEPATCH ...
./x86/Kconfig:171:      select HAVE_LIVEPATCH ...
./s390/Kconfig:161:     select HAVE_LIVEPATCH

arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_RELIABLE_STACKTRACE"                                                                                                                                                                     
./powerpc/Kconfig:223:  select HAVE_RELIABLE_STACKTRACE ...
./x86/Kconfig:189:      select HAVE_RELIABLE_STACKTRACE ...
./Kconfig:690:config HAVE_RELIABLE_STACKTRACE

klp_have_reliable_stack() will guard against loading of livepatching
module on s390, for the same reason being that HAVE_RELIABLE_STACKTRACE
is not set. My explanation is purely based on the above grep output
on Kconfig files, which might be partial. Am I missing something here?

> > 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:

[...]

> > 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);
> 
> On the other hand, I like this change. So we have two options, I think. 
> We can apply this and wait if someone complains (because of s390x 
> testing), or we can wait for the full support of s390x and then enforce 
> it.

Thanks, I am ok with either of the options.  We could enforce the config
dependency, in case the above assumption in regard to s390 is correct.

Thanks,
Kamalesh

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

[Miroslav Benes]

On Fri, 8 Feb 2019, Kamalesh Babulal wrote:

> Hi Miroslav,
> 
> On Fri, Feb 08, 2019 at 10:24:21AM +0100, Miroslav Benes wrote:
> > Hi Kamalesh,
> > 
> > On Fri, 8 Feb 2019, Kamalesh Babulal wrote:
> > 
> > > 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.
> 
> [...]
> 
> > > 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?
> > 
> > if I am not mistaken, s390x is currently the only one which is supported 
> > (the redirection works) but has no reliable stacktraces (so far, it is my 
> > plan to take a look soon).
> > 
> > Theoretically, it could still work. We have the fake signal and we can 
> > force the remaining tasks (kthreads). It is not something to be used in 
> > production but it could make sense for a limited testing.
> 
> That was my understanding too, s390 doesn't set HAVE_RELIABLE_STACKTRACE.
> 
> (below output is right trimmed for readability)
> 
> arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_LIVEPATCH"                                                                                                                                                                               
> ./powerpc/Kconfig:209:  select HAVE_LIVEPATCH ...
> ./x86/Kconfig:171:      select HAVE_LIVEPATCH ...
> ./s390/Kconfig:161:     select HAVE_LIVEPATCH
> 
> arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_RELIABLE_STACKTRACE"                                                                                                                                                                     
> ./powerpc/Kconfig:223:  select HAVE_RELIABLE_STACKTRACE ...
> ./x86/Kconfig:189:      select HAVE_RELIABLE_STACKTRACE ...
> ./Kconfig:690:config HAVE_RELIABLE_STACKTRACE
> 
> klp_have_reliable_stack() will guard against loading of livepatching
> module on s390, for the same reason being that HAVE_RELIABLE_STACKTRACE
> is not set. My explanation is purely based on the above grep output
> on Kconfig files, which might be partial. Am I missing something here?

No, I don't think so.

I think I mentioned the theoretical possibility at the time the check was 
introduced and we came to the conclusion that it is worth it and we should 
enforce the reliable stacktraces.
 
> > > 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:
> 
> [...]
> 
> > > 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);
> > 
> > On the other hand, I like this change. So we have two options, I think. 
> > We can apply this and wait if someone complains (because of s390x 
> > testing), or we can wait for the full support of s390x and then enforce 
> > it.

Scratch this. It is enforced even now.
 
> Thanks, I am ok with either of the options.  We could enforce the config
> dependency, in case the above assumption in regard to s390 is correct.

Yes, I think it is a nice cleanup.

Miroslav

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

[Petr Mladek]

On Fri 2019-02-08 11:50:05, Kamalesh Babulal wrote:
> 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

Looks like a nice cleanup.

> 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:

Hmm, I see the following in kernel/trace/Kconfig:

config TRACING
[...]
	select STACKTRACE if STACKTRACE_SUPPORT

It seems that the depency is not guaranted. Or do I miss anything?

Anyway, it is pretty indirect. I would prefer to add dependency
on STACKTRACE explicitly into config LIVEPATCH.

Best Regards,
Petr

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

[Kamalesh Babulal]

On Fri, Feb 08, 2019 at 10:34:45AM +0100, Petr Mladek wrote:
> On Fri 2019-02-08 11:50:05, Kamalesh Babulal wrote:
> > 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.

[...]

> > 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
> 
> Looks like a nice cleanup.

Thanks

> 
> > 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:
> 
> Hmm, I see the following in kernel/trace/Kconfig:
> 
> config TRACING
> [...]
> 	select STACKTRACE if STACKTRACE_SUPPORT
> 
> It seems that the depency is not guaranted. Or do I miss anything?

I should have tried drawing the config dependency of CONFIG_STACKTRACE
in the mail.  It would saved me from nagivating wrong indirections in
tricky Kconfigs. You are right, CONFIG_STACKTRACE is not selected via
CONFIG_DYNAMIC_FTRACE_WITH_REGS config dependency chain.

> 
> Anyway, it is pretty indirect. I would prefer to add dependency
> on STACKTRACE explicitly into config LIVEPATCH.

Agree, explicitly listing CONFIG_STACKTRACE as one of the dependencies
under config LIVEPATCH will guarantee the dependency is met.  On the
curious note, digging through the git logs, CONFIG_STACKTRACE_SUPPORT
is enabled by default on x86/PowerPC for quite long now.

Thanks,
Kamalesh