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