[PATCH] livepatch: Enforce reliable stack trace as config dependency

[PATCH] livepatch: Enforce reliable stack trace as config dependency

[Kamalesh Babulal]

While the consistency model was introduced, architectures without the
reliable stack trace implementation could use the immediate flag for
livepatching but with its own limitations.

After removal of the immediate flag by commit d0807da78e11
("livepatch: Remove immediate feature"), reliable stack trace became
enforcing dependency for livepatch support on any architecture. In
the current code, we ensure that the dependency is met when
enabling the patch during the module load.

This dependency check can be improved by moving it to the Kconfig,
which disables the support for livepatching in the kernel for unmet
dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
STACKTRACE under config LIVEPATCH, it also removes the
klp_have_reliable_stack() function.

Loading a livepatching module on an architecture where reliable
stack trace is yet to be implemented, the user should see:

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

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

[pmladek@suse.com: Suggested to explicitly add CONFIG_STACKTRACE under
 config LIVEPATCH]
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jiri Kosina <jikos@kernel.org>
---
Patch is based on a087cdd4073b (origin/for-5.1/atomic-replace) branch

 include/linux/livepatch.h | 6 ------
 kernel/livepatch/Kconfig  | 2 ++
 kernel/livepatch/core.c   | 6 ------
 3 files changed, 2 insertions(+), 12 deletions(-)

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..4e4e4fe040f5 100644
--- a/kernel/livepatch/Kconfig
+++ b/kernel/livepatch/Kconfig
@@ -10,6 +10,8 @@ config LIVEPATCH
 	depends on SYSFS
 	depends on KALLSYMS_ALL
 	depends on HAVE_LIVEPATCH
+	depends on STACKTRACE
+	depends on HAVE_RELIABLE_STACKTRACE
 	depends on !TRIM_UNUSED_KSYMS
 	help
 	  Say Y here if you want to support kernel live patching.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d1af69e9f0e3..a7a00478f6c3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1034,12 +1034,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 -EOPNOTSUPP;
-	}
-
-
 	mutex_lock(&klp_mutex);
 
 	ret = klp_init_patch_early(patch);
-- 
2.20.1

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Petr Mladek]

On Sat 2019-02-09 14:47:28, Kamalesh Babulal wrote:
> While the consistency model was introduced, architectures without the
> reliable stack trace implementation could use the immediate flag for
> livepatching but with its own limitations.
> 
> After removal of the immediate flag by commit d0807da78e11
> ("livepatch: Remove immediate feature"), reliable stack trace became
> enforcing dependency for livepatch support on any architecture. In
> the current code, we ensure that the dependency is met when
> enabling the patch during the module load.
> 
> This dependency check can be improved by moving it to the Kconfig,
> which disables the support for livepatching in the kernel for unmet
> dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
> STACKTRACE under config LIVEPATCH, it also removes the
> klp_have_reliable_stack() function.
> 
> Loading a livepatching module on an architecture where reliable
> stack trace is yet to be implemented, the user should see:
> 
> insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module format
> 
> ...
> [  286.453463] livepatch_sample: module is marked as livepatch module, but livepatch support is disabled
> 
> [pmladek@suse.com: Suggested to explicitly add CONFIG_STACKTRACE under
>  config LIVEPATCH]
> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Josh Poimboeuf]

On Sat, Feb 09, 2019 at 02:47:28PM +0530, Kamalesh Babulal wrote:
> While the consistency model was introduced, architectures without the
> reliable stack trace implementation could use the immediate flag for
> livepatching but with its own limitations.
> 
> After removal of the immediate flag by commit d0807da78e11
> ("livepatch: Remove immediate feature"), reliable stack trace became
> enforcing dependency for livepatch support on any architecture. In
> the current code, we ensure that the dependency is met when
> enabling the patch during the module load.
> 
> This dependency check can be improved by moving it to the Kconfig,
> which disables the support for livepatching in the kernel for unmet
> dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
> STACKTRACE under config LIVEPATCH, it also removes the
> klp_have_reliable_stack() function.
> 
> Loading a livepatching module on an architecture where reliable
> stack trace is yet to be implemented, the user should see:
> 
> insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module format
> 
> ...
> [  286.453463] livepatch_sample: module is marked as livepatch module, but livepatch support is disabled

Wouldn't the module fail to build in the first place, since
CONFIG_LIVEPATCH is disabled?

Anyway, I'm not sure about this approach.  This patch makes the s390
livepatch code no longer compilable, turning it into completely dead
code.  So if something changes in the s390 code which causes it to stop
compiling, nobody will notice.

I think I'd rather go in the opposite direction: allow the patches to be
loaded.  Then they can be forced, if needed.  That enables both compile
and runtime testing.  That way we don't make any backward progress,
until such arches get reliable stacktraces.

-- 
Josh

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Kamalesh Babulal]

On Mon, Feb 11, 2019 at 08:08:13AM -0600, Josh Poimboeuf wrote:
> On Sat, Feb 09, 2019 at 02:47:28PM +0530, Kamalesh Babulal wrote:
> > After removal of the immediate flag by commit d0807da78e11
> > ("livepatch: Remove immediate feature"), reliable stack trace became
> > enforcing dependency for livepatch support on any architecture. In
> > the current code, we ensure that the dependency is met when
> > enabling the patch during the module load.
> > 
> > This dependency check can be improved by moving it to the Kconfig,
> > which disables the support for livepatching in the kernel for unmet
> > dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
> > STACKTRACE under config LIVEPATCH, it also removes the
> > klp_have_reliable_stack() function.
> > 
> > Loading a livepatching module on an architecture where reliable
> > stack trace is yet to be implemented, the user should see:
> > 
> > insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module format
> > 
> > ...
> > [  286.453463] livepatch_sample: module is marked as livepatch module, but livepatch support is disabled
> 
> Wouldn't the module fail to build in the first place, since
> CONFIG_LIVEPATCH is disabled?

Yes, with this patch applied, the livepatch modules will fail to build.
The intention was to paste the output of a module load, marked as the
livepatch but without the reliable stack trace support. I used the
previously compiled livepatch sample module for the illustration but
yeah we would not have a functioning module build in the first place.

> 
> Anyway, I'm not sure about this approach.  This patch makes the s390
> livepatch code no longer compilable, turning it into completely dead
> code.  So if something changes in the s390 code which causes it to stop
> compiling, nobody will notice.

Fair point, we can drop this patch in favor of getting compile time
testing for s390.

> 
> I think I'd rather go in the opposite direction: allow the patches to be
> loaded.  Then they can be forced, if needed.  That enables both compile
> and runtime testing.  That way we don't make any backward progress,
> until such arches get reliable stacktraces.

klp_have_reliable_stack() enforces implementation of reliable
stack trace and run time testing will not be complete with we returning
after the check.  We can re-think about enforcing the dependency after
we have the reliable stack trace for s390.

-- 
Kamalesh

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Petr Mladek]

On Mon 2019-02-11 08:08:13, Josh Poimboeuf wrote:
> Anyway, I'm not sure about this approach.  This patch makes the s390
> livepatch code no longer compilable, turning it into completely dead
> code.  So if something changes in the s390 code which causes it to stop
> compiling, nobody will notice.

Good point. Well, it is only small win when a code is buildable
but it could not get really used. Also the amount of arch-specific
code is really minimal.

> I think I'd rather go in the opposite direction: allow the patches to be
> loaded.  Then they can be forced, if needed.  That enables both compile
> and runtime testing.  That way we don't make any backward progress,
> until such arches get reliable stacktraces.

Do you mean to convert the error into warning?

For example, the change below. Note that I did not mention
the possibility to force the transition by intention. It is risky
and people should not get used to it.

Heh, I think that this was the main reason why it was the error.
We did not want to get people used to forcing livepatches.


diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d1af69e9f0e3..8d9bce251516 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1035,11 +1035,10 @@ int klp_enable_patch(struct klp_patch *patch)
 		return -ENODEV;
 
 	if (!klp_have_reliable_stack()) {
-		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
-		return -EOPNOTSUPP;
+		pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
+		pr_warn("Only one livepatch can be installed.\n");
 	}
 
-
 	mutex_lock(&klp_mutex);
 
 	ret = klp_init_patch_early(patch);


Best Regards,
Petr

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Jiri Kosina]

On Tue, 12 Feb 2019, Petr Mladek wrote:

> > I think I'd rather go in the opposite direction: allow the patches to be
> > loaded.  Then they can be forced, if needed.  That enables both compile
> > and runtime testing.  That way we don't make any backward progress,
> > until such arches get reliable stacktraces.
> 
> Do you mean to convert the error into warning?
> 
> For example, the change below. Note that I did not mention
> the possibility to force the transition by intention. It is risky
> and people should not get used to it.
> 
> Heh, I think that this was the main reason why it was the error.
> We did not want to get people used to forcing livepatches.
> 
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d1af69e9f0e3..8d9bce251516 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1035,11 +1035,10 @@ int klp_enable_patch(struct klp_patch *patch)
>  		return -ENODEV;
>  
>  	if (!klp_have_reliable_stack()) {
> -		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> -		return -EOPNOTSUPP;
> +		pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> +		pr_warn("Only one livepatch can be installed.\n");
>  	}
>  
> -

This seems to have been lost.

I think we should take this aproach before Miroslav is ready with 
realiable stack traces for s390. At the same time, I'd suggest issuing a 
proper WARN() there instead of just pr_warn(). The kernel might be in a 
potentially funky state, so let's at least get the 'W' taint in place.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Jiri Kosina]

On Tue, 16 Apr 2019, Jiri Kosina wrote:

> > Do you mean to convert the error into warning?
> > 
> > For example, the change below. Note that I did not mention
> > the possibility to force the transition by intention. It is risky
> > and people should not get used to it.
> > 
> > Heh, I think that this was the main reason why it was the error.
> > We did not want to get people used to forcing livepatches.
> > 
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index d1af69e9f0e3..8d9bce251516 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -1035,11 +1035,10 @@ int klp_enable_patch(struct klp_patch *patch)
> >  		return -ENODEV;
> >  
> >  	if (!klp_have_reliable_stack()) {
> > -		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> > -		return -EOPNOTSUPP;
> > +		pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> > +		pr_warn("Only one livepatch can be installed.\n");
> >  	}
> >  
> > -
> 
> This seems to have been lost.
> 
> I think we should take this aproach before Miroslav is ready with 
> realiable stack traces for s390. At the same time, I'd suggest issuing a 
> proper WARN() there instead of just pr_warn(). The kernel might be in a 
> potentially funky state, so let's at least get the 'W' taint in place.

Ignore the above, there is nothing wrong with the kernel unless the patch 
is forced.

So we should be good taking it as-is. Petr, could you please send it with 
proper changelog and signoff? Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Josh Poimboeuf]

On Tue, Apr 16, 2019 at 01:47:30PM +0200, Jiri Kosina wrote:
> On Tue, 12 Feb 2019, Petr Mladek wrote:
> 
> > > I think I'd rather go in the opposite direction: allow the patches to be
> > > loaded.  Then they can be forced, if needed.  That enables both compile
> > > and runtime testing.  That way we don't make any backward progress,
> > > until such arches get reliable stacktraces.
> > 
> > Do you mean to convert the error into warning?
> > 
> > For example, the change below. Note that I did not mention
> > the possibility to force the transition by intention. It is risky
> > and people should not get used to it.
> > 
> > Heh, I think that this was the main reason why it was the error.
> > We did not want to get people used to forcing livepatches.
> > 
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index d1af69e9f0e3..8d9bce251516 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -1035,11 +1035,10 @@ int klp_enable_patch(struct klp_patch *patch)
> >  		return -ENODEV;
> >  
> >  	if (!klp_have_reliable_stack()) {
> > -		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> > -		return -EOPNOTSUPP;
> > +		pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> > +		pr_warn("Only one livepatch can be installed.\n");
> >  	}
> >  
> > -
> 
> This seems to have been lost.

Sorry, this must have gotten lost in my inbox - yes, something like the
above is what I had in mind.  Though instead of "one livepatch can be
installed" it might say that the patch transition may never complete.

BTW, might we want to consider adding a way to say "this patch doesn't
need the consistency model", which just applies the patch immediately
like we used to?  Like patch->simple = true?  Then we could easily
support all arches for basic patches.

> I think we should take this aproach before Miroslav is ready with 
> realiable stack traces for s390. At the same time, I'd suggest issuing a 
> proper WARN() there instead of just pr_warn(). The kernel might be in a 
> potentially funky state, so let's at least get the 'W' taint in place.

I don't think it would be in a dangerous state, because
save_stack_trace_tsk_reliable() will return -ENOSYS and the patch will
remain in transition forever because the signaling doesn't work for
kthreads.  So I don't think a warning is necessary.  In fact we may want
to remove the warning in the generic version of
save_stack_trace_tsk_reliable().

-- 
Josh

Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency

[Miroslav Benes]

On Tue, 16 Apr 2019, Josh Poimboeuf wrote:

> On Tue, Apr 16, 2019 at 01:47:30PM +0200, Jiri Kosina wrote:
> > On Tue, 12 Feb 2019, Petr Mladek wrote:
> > 
> > > > I think I'd rather go in the opposite direction: allow the patches to be
> > > > loaded.  Then they can be forced, if needed.  That enables both compile
> > > > and runtime testing.  That way we don't make any backward progress,
> > > > until such arches get reliable stacktraces.
> > > 
> > > Do you mean to convert the error into warning?
> > > 
> > > For example, the change below. Note that I did not mention
> > > the possibility to force the transition by intention. It is risky
> > > and people should not get used to it.
> > > 
> > > Heh, I think that this was the main reason why it was the error.
> > > We did not want to get people used to forcing livepatches.
> > > 
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index d1af69e9f0e3..8d9bce251516 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -1035,11 +1035,10 @@ int klp_enable_patch(struct klp_patch *patch)
> > >  		return -ENODEV;
> > >  
> > >  	if (!klp_have_reliable_stack()) {
> > > -		pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> > > -		return -EOPNOTSUPP;
> > > +		pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> > > +		pr_warn("Only one livepatch can be installed.\n");
> > >  	}
> > >  
> > > -
> > 
> > This seems to have been lost.
> 
> Sorry, this must have gotten lost in my inbox - yes, something like the
> above is what I had in mind.  Though instead of "one livepatch can be
> installed" it might say that the patch transition may never complete.

Sounds better to me too.
 
> BTW, might we want to consider adding a way to say "this patch doesn't
> need the consistency model", which just applies the patch immediately
> like we used to?  Like patch->simple = true?  Then we could easily
> support all arches for basic patches.

I'd rather not return to immediate. There was a bug (commit d0807da78e11 
("livepatch: Remove immediate feature") explains it), it made the code 
complicated and it was impossible to disable patches/remove modules with 
that. After all, the consistency model gives us not only the consistency, 
but also assurance that all tasks were migrated outside of patched 
functions.

> > I think we should take this aproach before Miroslav is ready with 
> > realiable stack traces for s390. At the same time, I'd suggest issuing a 
> > proper WARN() there instead of just pr_warn(). The kernel might be in a 
> > potentially funky state, so let's at least get the 'W' taint in place.
> 
> I don't think it would be in a dangerous state, because
> save_stack_trace_tsk_reliable() will return -ENOSYS and the patch will
> remain in transition forever because the signaling doesn't work for
> kthreads.  So I don't think a warning is necessary.  In fact we may want
> to remove the warning in the generic version of
> save_stack_trace_tsk_reliable().

I would not mind.

Miroslav