linux-kernel - Re: [RFC PATCH 03/14] kthread: Add drain_kthread

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, 29 Jul 2015 11:03:33 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Josh Triplett <josh@...htriplett.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>, Borislav Petkov <bp@...e.de>,
	Michal Hocko <mhocko@...e.cz>, linux-mm@...ck.org,
	Vlastimil Babka <vbabka@...e.cz>,
	live-patching@...r.kernel.org, linux-api@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 03/14] kthread: Add drain_kthread_worker()

Hello, Petr.

On Wed, Jul 29, 2015 at 12:04:57PM +0200, Petr Mladek wrote:
> > I'm not sure full-on chained work detection is necessary here.
> > kthread worker's usages tend to be significantly simpler and draining
> > is only gonna be used for destruction.
> 
> I think that it might be useful to detect bugs when someone
> depends on the worker when it is being destroyed. For example,
> I tried to convert "khubd" kthread and there was not easy to
> double check that this worked as expected.
> 
> I actually think about replacing
> 
>     WARN_ON_ONCE(!is_chained_work(worker)))
> 
> with
> 
>     WARN_ON(!is_chained_work(worker)))
> 
> in queue_kthread_work, so that we get the warning for all misused
> workers.

This is a partial soluation no matter what you do especially for
destruction path as there's nothing which prevents draining and
destruction winning the race and then external queueing coming in
afterwards.  For use-after-free, slab debug should work pretty well.
I really don't think we need anything special here.

> > > +	while (!list_empty(&worker->work_list)) {
> > > +		/*
> > > +		 * Unlock, so we could move forward. Note that queuing
> > > +		 * is limited by @nr_drainers > 0.
> > > +		 */
> > > +		spin_unlock_irq(&worker->lock);
> > > +
> > > +		flush_kthread_worker(worker);
> > > +
> > > +		if (++flush_cnt == 10 ||
> > > +		    (flush_cnt % 100 == 0 && flush_cnt <= 1000))
> > > +			pr_warn("kthread worker %s: drain_kthread_worker() isn't complete after %u tries\n",
> > > +				worker->task->comm, flush_cnt);
> > > +
> > > +		spin_lock_irq(&worker->lock);
> > > +	}
> > 
> > I'd just do something like WARN_ONCE(flush_cnt++ > 10, "kthread worker: ...").
> 
> This would print the warning only for one broken worker. But I do not
> have strong opinion about it.

I really think that'd be a good enough protection here.  It's
indicative an outright kernel bug and things tend to go awry and/or
badly reported after the initial failure anyway.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/