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