linux-kernel - Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Apr 2019 14:49:20 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Jiri Kosina <jikos@...nel.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Miroslav Benes <mbenes@...e.cz>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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 <pmladek@...e.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