linux-kernel - Re: [PATCH] iio: fix sched WARNING "do not call blocking ops when !TASK

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: <20160804094113.GA25537@localhost>
Date:	Thu, 4 Aug 2016 17:41:14 +0800
From:	Brian Norris <briannorris@...omium.org>
To:	Lars-Peter Clausen <lars@...afoo.de>
Cc:	Jonathan Cameron <jic23@...nel.org>,
	Hartmut Knaack <knaack.h@....de>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Guenter Roeck <linux@...ck-us.net>,
	Brian Norris <computersforpeace@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH] iio: fix sched WARNING "do not call blocking ops when
 !TASK_RUNNING"

On Thu, Aug 04, 2016 at 10:45:39AM +0200, Lars-Peter Clausen wrote:
> > @@ -132,10 +133,13 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
> >  		to_wait = min_t(size_t, n / datum_size, rb->watermark);
> >  
> >  	do {
> > -		ret = wait_event_interruptible(rb->pollq,
> > -		      iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size));
> > -		if (ret)
> > -			return ret;
> > +		add_wait_queue(&rb->pollq, &wait);
> > +		while (!iio_buffer_ready(indio_dev, rb, to_wait,
> > +					 n / datum_size)) {
> > +			wait_woken(&wait, TASK_INTERRUPTIBLE,
> > +				   MAX_SCHEDULE_TIMEOUT);
> 
> We loose the ability to break out from this loop by sending a signal to the
> task. This needs something like
> 
> 	if (signal_pending(current)) {
> 		ret = -ERESTARTSYS;
> 		break;
> 	}
> 
> before the wait_woken()

Sounds good.

> And as a minor improvement I'd also move the
> add_wait_queue()/remove_wait_queue() outside of the outer loop.

Sure.

> And then
> just if (!iio_buffer_ready(...)) continue; rather than having the inner
> loop. This should slightly simplify the flow.

Perhaps I'm not gathering your meaning here, but wouldn't that turn this
into a spin loop, waiting for iio_buffer_ready()? i.e.:

	do {
		if (!iio_buffer_ready(...))
			continue; // we shouldn't just hammer
				  // iio_buffer_ready(), should we?

		wait_woken(...);
		...
	};

> Just make sure to replace the
> returns in the loop with a break so remove_wait_queue() has a chance to run.
> 
> 
> > +		}
> > +		remove_wait_queue(&rb->pollq, &wait);
> >  
> >  		if (!indio_dev->info)
> >  			return -ENODEV;
> > 
> 

Brian