[PATCH 0/3] livepatch/stacktrace: Clean up of reliable stacktrace errors

[PATCH 0/3] livepatch/stacktrace: Clean up of reliable stacktrace errors

[Petr Mladek]

I ended with three patches in the end. The first one is the same
as before [1]. Then I added two patches for handling of the stacktrace
error messages.
 
[1] https://lkml.kernel.org/r/20190418112936.13295-1-pmladek@suse.com

Petr Mladek (3):
  livepatch: Convert error about unsupported reliable stacktrace into a
    warning
  stacktrace: Remove superfluous WARN_ONCE() from
    save_stack_trace_tsk_reliable()
  livepatch: Cleanup message handling in klp_try_switch_task()

 kernel/livepatch/core.c       |  5 ++---
 kernel/livepatch/transition.c | 19 ++++++++++++++-----
 kernel/stacktrace.c           |  1 -
 3 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.16.4

[PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning

[Petr Mladek]

The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
that any livepatch was refused when reliable stacktraces were not supported
on the given architecture.

The limitation is too strong. User space processes are safely migrated
even when entering or leaving the kernel. Kthreads transition would
need to get forced. But it is safe when:

   + The livepatch does not change the semantic of the code.
   + Callbacks do not depend on a safely finished transition.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..14f33ab6c583 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1003,11 +1003,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("The livepatch transition may never complete.\n");
 	}
 
-
 	mutex_lock(&klp_mutex);
 
 	ret = klp_init_patch_early(patch);
-- 
2.16.4

Re: [PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning

[Josh Poimboeuf]

On Wed, Apr 24, 2019 at 10:55:48AM +0200, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
> 
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
> 
>    + The livepatch does not change the semantic of the code.
>    + Callbacks do not depend on a safely finished transition.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..14f33ab6c583 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1003,11 +1003,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("The livepatch transition may never complete.\n");
>  	}
>  
> -
>  	mutex_lock(&klp_mutex);
>  
>  	ret = klp_init_patch_early(patch);
> -- 
> 2.16.4
> 

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning

[Miroslav Benes]

On Wed, 24 Apr 2019, Petr Mladek wrote:

> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
> 
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
> 
>    + The livepatch does not change the semantic of the code.
>    + Callbacks do not depend on a safely finished transition.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning

[Kamalesh Babulal]

On Wed, Apr 24, 2019 at 10:55:48AM +0200, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
> 
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
> 
>    + The livepatch does not change the semantic of the code.
>    + Callbacks do not depend on a safely finished transition.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

-- 
Kamalesh

Re: [PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning

[Petr Mladek]

On Wed 2019-04-24 10:55:48, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
> 
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
> 
>    + The livepatch does not change the semantic of the code.
>    + Callbacks do not depend on a safely finished transition.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..14f33ab6c583 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1003,11 +1003,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("The livepatch transition may never complete.\n");
>  	}
>  
> -
>  	mutex_lock(&klp_mutex);
>  
>  	ret = klp_init_patch_early(patch);
> -- 
> 2.16.4

I have committed this patch into for-5.2/core branch.

Best Regards,
Petr

PS: I am going to resend 2nd patch separately with more people
    interested into kernel/stacktrace.c.

    Also I am going to send two separate patches instead of
    the 3rd one (complete warning removal, static err_buf).

[PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Petr Mladek]

WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
The only current user klp_check_stack() writes its own warning when
-ENOSYS is returned.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/stacktrace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index f8edee9c792d..83ac0ac5ffd9 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -74,6 +74,5 @@ __weak int
 save_stack_trace_tsk_reliable(struct task_struct *tsk,
 			      struct stack_trace *trace)
 {
-	WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
 	return -ENOSYS;
 }
-- 
2.16.4

Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Petr Mladek]

On Wed 2019-04-24 10:55:49, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

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

Best Regards,
Petr

Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Josh Poimboeuf]

Adding Thomas because this might slightly conflict with some of the
stacktrace.c improvements he's working on.

On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/stacktrace.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index f8edee9c792d..83ac0ac5ffd9 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -74,6 +74,5 @@ __weak int
>  save_stack_trace_tsk_reliable(struct task_struct *tsk,
>  			      struct stack_trace *trace)
>  {
> -	WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
>  	return -ENOSYS;
>  }
> -- 
> 2.16.4
> 

-- 
Josh

Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Josh Poimboeuf]

On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/stacktrace.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index f8edee9c792d..83ac0ac5ffd9 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -74,6 +74,5 @@ __weak int
>  save_stack_trace_tsk_reliable(struct task_struct *tsk,
>  			      struct stack_trace *trace)
>  {
> -	WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
>  	return -ENOSYS;
>  }
> -- 
> 2.16.4
> 

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Miroslav Benes]

On Wed, 24 Apr 2019, Petr Mladek wrote:

> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()

[Kamalesh Babulal]

On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

-- 
Kamalesh

[PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Petr Mladek]

WARN_ON_ONCE() could not be called safely under rq lock because
of console deadlock issues. Fortunately, simple printk_deferred()
is enough because the warning is printed from a well defined
location and context.

Also klp_try_switch_task() is called under klp_mutex.
Therefore, the buffer for debug messages could be static.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/transition.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 9c89ae8b337a..e8183d18227f 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
 static int klp_check_stack(struct task_struct *task, char *err_buf)
 {
 	static unsigned long entries[MAX_STACK_ENTRIES];
+	static int enosys_warned;
 	struct stack_trace trace;
 	struct klp_object *obj;
 	struct klp_func *func;
@@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
 	trace.nr_entries = 0;
 	trace.max_entries = MAX_STACK_ENTRIES;
 	trace.entries = entries;
+
 	ret = save_stack_trace_tsk_reliable(task, &trace);
-	WARN_ON_ONCE(ret == -ENOSYS);
+	if (ret == -ENOSYS) {
+		if (!enosys_warned) {
+			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
+					__func__);
+			enosys_warned = 1;
+		}
+		return ret;
+	}
 	if (ret) {
 		snprintf(err_buf, STACK_ERR_BUF_SIZE,
 			 "%s: %s:%d has an unreliable stack\n",
@@ -297,11 +306,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
  */
 static bool klp_try_switch_task(struct task_struct *task)
 {
+	static char err_buf[STACK_ERR_BUF_SIZE];
 	struct rq *rq;
 	struct rq_flags flags;
 	int ret;
 	bool success = false;
-	char err_buf[STACK_ERR_BUF_SIZE];
 
 	err_buf[0] = '\0';
 
@@ -336,15 +345,15 @@ static bool klp_try_switch_task(struct task_struct *task)
 	task_rq_unlock(rq, task, &flags);
 
 	/*
-	 * Due to console deadlock issues, pr_debug() can't be used while
-	 * holding the task rq lock.  Instead we have to use a temporary buffer
+	 * printk_deferred() need to be used under rq lock to avoid
+	 * console deadlock issues.  But it does not support the dynamic
+	 * debug feature.  Instead we have to use a temporary buffer
 	 * and print the debug message after releasing the lock.
 	 */
 	if (err_buf[0] != '\0')
 		pr_debug("%s", err_buf);
 
 	return success;
-
 }
 
 /*
-- 
2.16.4

Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Jiri Kosina]

On Wed, 24 Apr 2019, Petr Mladek wrote:

> WARN_ON_ONCE() could not be called safely under rq lock because
> of console deadlock issues. Fortunately, simple printk_deferred()
> is enough because the warning is printed from a well defined
> location and context.
> 
> Also klp_try_switch_task() is called under klp_mutex.
> Therefore, the buffer for debug messages could be static.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/transition.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 9c89ae8b337a..e8183d18227f 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
>  static int klp_check_stack(struct task_struct *task, char *err_buf)
>  {
>  	static unsigned long entries[MAX_STACK_ENTRIES];
> +	static int enosys_warned;
>  	struct stack_trace trace;
>  	struct klp_object *obj;
>  	struct klp_func *func;
> @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
>  	trace.nr_entries = 0;
>  	trace.max_entries = MAX_STACK_ENTRIES;
>  	trace.entries = entries;
> +
>  	ret = save_stack_trace_tsk_reliable(task, &trace);
> -	WARN_ON_ONCE(ret == -ENOSYS);
> +	if (ret == -ENOSYS) {
> +		if (!enosys_warned) {
> +			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> +					__func__);
> +			enosys_warned = 1;

... abusing the fact that you are also printk maintainer :) ... looking at 
the above, wouldn't it make sense to introduce generic 
printk_deferred_once() instead?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Petr Mladek]

On Wed 2019-04-24 12:41:00, Jiri Kosina wrote:
> On Wed, 24 Apr 2019, Petr Mladek wrote:
> 
> > WARN_ON_ONCE() could not be called safely under rq lock because
> > of console deadlock issues. Fortunately, simple printk_deferred()
> > is enough because the warning is printed from a well defined
> > location and context.
> > 
> > Also klp_try_switch_task() is called under klp_mutex.
> > Therefore, the buffer for debug messages could be static.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  kernel/livepatch/transition.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 9c89ae8b337a..e8183d18227f 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> >  static int klp_check_stack(struct task_struct *task, char *err_buf)
> >  {
> >  	static unsigned long entries[MAX_STACK_ENTRIES];
> > +	static int enosys_warned;
> >  	struct stack_trace trace;
> >  	struct klp_object *obj;
> >  	struct klp_func *func;
> > @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> >  	trace.nr_entries = 0;
> >  	trace.max_entries = MAX_STACK_ENTRIES;
> >  	trace.entries = entries;
> > +
> >  	ret = save_stack_trace_tsk_reliable(task, &trace);
> > -	WARN_ON_ONCE(ret == -ENOSYS);
> > +	if (ret == -ENOSYS) {
> > +		if (!enosys_warned) {
> > +			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > +					__func__);
> > +			enosys_warned = 1;
> 
> ... abusing the fact that you are also printk maintainer :) ... looking at 
> the above, wouldn't it make sense to introduce generic 
> printk_deferred_once() instead?

Yeah, I thought about it. Also pr_debug_deferred() would be useful for
the other messages passed via err_buf.

Sigh, printk_deferred() is whack-a-mole approach. We use it because
we do not have anything better for the deadlocks. I am a bit scared
to add more wrappers because it might encourage people to use it
more widely, e.g. to avoid softlockups or secure fast paths.

On the other hand, it still looks better to find all occurrences
easily instead of hiding them into a custom workarounds.

OK, I am going to prepare new patchset and involve printk
people into it.

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Josh Poimboeuf]

On Wed, Apr 24, 2019 at 10:55:50AM +0200, Petr Mladek wrote:
> WARN_ON_ONCE() could not be called safely under rq lock because
> of console deadlock issues. Fortunately, simple printk_deferred()
> is enough because the warning is printed from a well defined
> location and context.
> 
> Also klp_try_switch_task() is called under klp_mutex.
> Therefore, the buffer for debug messages could be static.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/transition.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 9c89ae8b337a..e8183d18227f 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
>  static int klp_check_stack(struct task_struct *task, char *err_buf)
>  {
>  	static unsigned long entries[MAX_STACK_ENTRIES];
> +	static int enosys_warned;
>  	struct stack_trace trace;
>  	struct klp_object *obj;
>  	struct klp_func *func;
> @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
>  	trace.nr_entries = 0;
>  	trace.max_entries = MAX_STACK_ENTRIES;
>  	trace.entries = entries;
> +
>  	ret = save_stack_trace_tsk_reliable(task, &trace);
> -	WARN_ON_ONCE(ret == -ENOSYS);
> +	if (ret == -ENOSYS) {
> +		if (!enosys_warned) {
> +			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> +					__func__);
> +			enosys_warned = 1;
> +		}
> +		return ret;
> +	}

We already have a similar printk in patch 1, so is this warning really
needed?

-- 
Josh

Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Miroslav Benes]

On Wed, 24 Apr 2019, Josh Poimboeuf wrote:

> On Wed, Apr 24, 2019 at 10:55:50AM +0200, Petr Mladek wrote:
> > WARN_ON_ONCE() could not be called safely under rq lock because
> > of console deadlock issues. Fortunately, simple printk_deferred()
> > is enough because the warning is printed from a well defined
> > location and context.
> > 
> > Also klp_try_switch_task() is called under klp_mutex.
> > Therefore, the buffer for debug messages could be static.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  kernel/livepatch/transition.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 9c89ae8b337a..e8183d18227f 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> >  static int klp_check_stack(struct task_struct *task, char *err_buf)
> >  {
> >  	static unsigned long entries[MAX_STACK_ENTRIES];
> > +	static int enosys_warned;
> >  	struct stack_trace trace;
> >  	struct klp_object *obj;
> >  	struct klp_func *func;
> > @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> >  	trace.nr_entries = 0;
> >  	trace.max_entries = MAX_STACK_ENTRIES;
> >  	trace.entries = entries;
> > +
> >  	ret = save_stack_trace_tsk_reliable(task, &trace);
> > -	WARN_ON_ONCE(ret == -ENOSYS);
> > +	if (ret == -ENOSYS) {
> > +		if (!enosys_warned) {
> > +			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > +					__func__);
> > +			enosys_warned = 1;
> > +		}
> > +		return ret;
> > +	}
> 
> We already have a similar printk in patch 1, so is this warning really
> needed?

I don't think so. pr_warn() in klp_enable_patch() should be enough in my 
opinion.

However,

if (ret == -ENOSYS)
	return ret;

would be justified, wouldn't it?

Miroslav

Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()

[Kamalesh Babulal]

On Wed, Apr 24, 2019 at 08:48:58PM +0200, Miroslav Benes wrote:
> On Wed, 24 Apr 2019, Josh Poimboeuf wrote:
> 
[...]
> > >  	ret = save_stack_trace_tsk_reliable(task, &trace);
> > > -	WARN_ON_ONCE(ret == -ENOSYS);
> > > +	if (ret == -ENOSYS) {
> > > +		if (!enosys_warned) {
> > > +			printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > > +					__func__);
> > > +			enosys_warned = 1;
> > > +		}
> > > +		return ret;
> > > +	}
> > 
> > We already have a similar printk in patch 1, so is this warning really
> > needed?
> 
> I don't think so. pr_warn() in klp_enable_patch() should be enough in my 
> opinion.
> 
> However,
> 
> if (ret == -ENOSYS)
> 	return ret;
> 
> would be justified, wouldn't it?
> 

Probably an one line comment on why we return, will be helpful.

-- 
Kamalesh