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