linux-kernel - Re: [PATCH v4 07/22] kthread: Detect when a kthread work is used by more workers

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]

Message-ID: <20160216163839.GP3305@pathway.suse.cz>
Date:	Tue, 16 Feb 2016 17:38:39 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	Tejun Heo <tj@...nel.org>
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>, linux-api@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/22] kthread: Detect when a kthread work is used by
 more workers

On Mon 2016-01-25 13:57:47, Tejun Heo wrote:
> On Mon, Jan 25, 2016 at 04:44:56PM +0100, Petr Mladek wrote:
> > +static void insert_kthread_work_sanity_check(struct kthread_worker *worker,
> > +					       struct kthread_work *work)
> > +{
> > +	lockdep_assert_held(&worker->lock);
> > +	WARN_ON_ONCE(!irqs_disabled());
> 
> Isn't worker->lock gonna be a irq-safe lock?  If so, why would this
> need to be tested separately?

I see. I'll remove it.

> > +	WARN_ON_ONCE(!list_empty(&work->node));
> > +	/* Do not use a work with more workers, see queue_kthread_work() */
> > +	WARN_ON_ONCE(work->worker && work->worker != worker);
> > +}
> 
> Is this sanity check function gonna be used from multiple places?

It will be reused when implementing __queue_delayed_kthread_work().
We will want to do these checks also before setting the timer.
See the 8th patch, e.g. at
http://thread.gmane.org/gmane.linux.kernel.mm/144964/focus=144973


> >  /* insert @work before @pos in @worker */
> >  static void insert_kthread_work(struct kthread_worker *worker,
> > -			       struct kthread_work *work,
> > -			       struct list_head *pos)
> > +				struct kthread_work *work,
> > +				struct list_head *pos)
> >  {
> > -	lockdep_assert_held(&worker->lock);
> > +	insert_kthread_work_sanity_check(worker, work);
> >  
> >  	list_add_tail(&work->node, pos);
> >  	work->worker = worker;
> > @@ -717,6 +730,15 @@ static void insert_kthread_work(struct kthread_worker *worker,
> >   * Queue @work to work processor @task for async execution.  @task
> >   * must have been created with kthread_worker_create().  Returns %true
> >   * if @work was successfully queued, %false if it was already pending.
> > + *
> > + * Never queue a work into a worker when it is being processed by another
> > + * one. Otherwise, some operations, e.g. cancel or flush, will not work
> > + * correctly or the work might run in parallel. This is not enforced
> > + * because it would make the code too complex. There are only warnings
> > + * printed when such a situation is detected.
> 
> I'm not sure the above paragraph adds much.  It isn't that accurate to
> begin with as what's being disallowed is larger scope than the above.
> Isn't the paragraph below enough?

Makes sense. I'll remove it.

> > + * Reinitialize the work if it needs to be used by another worker.
> > + * For example, when the worker was stopped and started again.
> >   */
> >  bool queue_kthread_work(struct kthread_worker *worker,
> >  			struct kthread_work *work)

Thanks,
Petr