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@xxxxxxxxxxxxxxxx>
> > > > >
> > > > > 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
