Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()
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, );
> > > - 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
Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()
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
> > ---
> > 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, );
> > - 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()
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
> ---
> 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, );
> - 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()
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
> > ---
> > 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, );
> > - 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()
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
> ---
> 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, );
> - 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
[PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()
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
---
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, );
- 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, );
/*
-* 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
