iio:buffer: polling unregistered device stalls user process

Using iio_device_unregister() on a device currently in use may stall userspace process polling for data availability (poll syscall).

If device has vanished before running the iio_buffer_fileops poll hook, the latter will return empty poll event mask. Process will be stalled waiting for events that will never come (if no timeout specified).

This patch ensures iio_buffer_poll() returns POLLERR if device has just been unregistered in order to properly notify userspace process something wrong happened (such as removable device unplugged).

Signed-off-by Gregor Boirie <[email protected]>

---
 drivers/iio/industrialio-buffer.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/iio/industrialio-buffer.c +10 -2

index 90462fc..aad5159 100644
@@ -155,6 +155,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
  *		a wait queue
  *
  * Return: (POLLIN | POLLRDNORM) if data is available for reading
+ *	   POLLERR if device was unregistered in our back
  *	   or 0 for other cases
  */
 unsigned int iio_buffer_poll(struct file *filp,

@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
 {
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
+	bool rdy;
 
 	if (!indio_dev->info)
-		return 0;
+		return POLLERR;
 
 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+	rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+	if (!indio_dev->info)
+		return POLLERR;
+
+	if (rdy)
 		return POLLIN | POLLRDNORM;
+
 	return 0;
 }
 

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 169

Quoted 9 lines

drivers/iio/industrialio-buffer.c +2 -1

@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
 {
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
+	bool rdy;
 
 	if (!indio_dev->info)
-		return 0;
+		return POLLERR;

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 175

Quoted 7 lines

drivers/iio/industrialio-buffer.c +4 -1

 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+	rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+	if (!indio_dev->info)
+		return POLLERR;

Hi,

I had some more comments inline in v1, please see below.

Quoted 6 lines · Jonathan Cameron

Quoted 6 lines

drivers/iio/industrialio-buffer.c +1 -0

@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
  {
  	struct iio_dev *indio_dev = filp->private_data;
  	struct iio_buffer *rb = indio_dev->buffer;
+	bool rdy;
 

Quoted 3 lines

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 169

Quoted 3 lines

drivers/iio/industrialio-buffer.c +1 -1

  	if (!indio_dev->info)
-		return 0;
+		return POLLERR;

I've had a look at what other subsystems do and input returns POLLERR | POLLHUP. Maybe we should mirror this.

Quoted 8 lines

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 175

Quoted 8 lines

drivers/iio/industrialio-buffer.c +4 -1

  	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+	rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+	if (!indio_dev->info)
+		return POLLERR;

Why check indio_dev->info twice though, since there is no locking involved the second check does gain us anything in terms of race condition avoidance.

I think we should have one check after the poll_wait() call. If the device is unregistered we wake the pollq. So adding it there should make sure that the poll() callback is called again if the device is unregistered after the check.

I'm not sure though if we'd need a explicit memory barrier or whether poll_wait() can act as a implicit one. But probably we are ok, given that poll_wait() should take a lock.

To be sure that the code is race free we need a guarantee that there is strict ordering between the indio_dev->info = NULL and wake_up() in unregister as well as between poll_wait() and the if (indio_dev->info) check in poll().

The scenario is basically

CPU0 CPU1
---------------------------------------------------------
poll_wait() indio_dev->info = NULL
if (!indio_dev->info) wake_up()

If strict ordering is not observed for either side we could end up missing the unregister and poll() will block forever just as before.

Hi,

Quoted 5 lines · Lars-Peter Clausen

Hi,

I had some more comments inline in v1, please see below.

On 09/10/2016 05:44 PM, Jonathan Cameron wrote:

Quoted 10 lines

Quoted 8 lines

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 168

Quoted 8 lines

drivers/iio/industrialio-buffer.c +2 -1

@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
  {
      struct iio_dev *indio_dev = filp->private_data;
      struct iio_buffer *rb = indio_dev->buffer;
+    bool rdy;
        if (!indio_dev->info)
-        return 0;
+        return POLLERR;

I've had a look at what other subsystems do and input returns POLLERR | POLLHUP. Maybe we should mirror this.

Well, some input device only don't IFAIK (I'm no expert on this) as POLLHUP might be focused on output errors only.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/iio/industrialio-buffer.c +4 -1

        poll_wait(filp, &rb->pollq, wait);
-    if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+    rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+    if (!indio_dev->info)
+        return POLLERR;

Quoted 2 lines

Why check indio_dev->info twice though, since there is no locking involved the second check does gain us anything in terms of race condition avoidance.

I took iio_buffer_read_first_n_outer() as a reference : the check of indio_dev->info is performed at least twice :
* once at function start
* each time wait_event_interruptible() returns.

I thought it would be a good starting point.

Quoted 4 lines

I think we should have one check after the poll_wait() call. If the device is unregistered we wake the pollq. So adding it there should make sure that the poll() callback is called again if the device is unregistered after the check.

No risk that an "exotic" userspace process perform 2 sys_poll() in the time between iio_device_unregister() and iio_device_free() ?
I mean : am I wrong in thinking that the filep is still valid as long as the fd has
not been released completly ?

Quoted 21 lines

I'm not sure though if we'd need a explicit memory barrier or whether poll_wait() can act as a implicit one. But probably we are ok, given that poll_wait() should take a lock.

To be sure that the code is race free we need a guarantee that there is strict ordering between the indio_dev->info = NULL and wake_up() in unregister as well as between poll_wait() and the if (indio_dev->info) check in poll().

The scenario is basically

CPU0 CPU1
---------------------------------------------------------
poll_wait() indio_dev->info = NULL
if (!indio_dev->info) wake_up()

If strict ordering is not observed for either side we could end up missing the unregister and poll() will block forever just as before.

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 18 lines · Gregor Boirie

Hi,

Quoted 5 lines · Lars-Peter Clausen

Hi,

I had some more comments inline in v1, please see below.

On 09/10/2016 05:44 PM, Jonathan Cameron wrote:

Quoted 10 lines

Quoted 8 lines

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 168

Quoted 8 lines

drivers/iio/industrialio-buffer.c +2 -1

@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
  {
      struct iio_dev *indio_dev = filp->private_data;
      struct iio_buffer *rb = indio_dev->buffer;
+    bool rdy;
        if (!indio_dev->info)
-        return 0;
+        return POLLERR;

I've had a look at what other subsystems do and input returns POLLERR | POLLHUP. Maybe we should mirror this.

Quoted 2 lines

Well, some input device only don't IFAIK (I'm no expert on this) as POLLHUP might be focused on output errors only.

The way I understand it is that the 'output only' description for POLLHUP means that you can't pass POLLHUP as a event to listen for. A POLLHUP will always wakeup the poll() and be set in the revents field. It does not meant that is only set for output devices.

Quoted 15 lines

Quoted 10 lines

Quoted 8 lines

drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 175

Quoted 8 lines

drivers/iio/industrialio-buffer.c +4 -1

        poll_wait(filp, &rb->pollq, wait);
-    if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+    rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+    if (!indio_dev->info)
+        return POLLERR;

Why check indio_dev->info twice though, since there is no locking involved the second check does gain us anything in terms of race condition avoidance.

I took iio_buffer_read_first_n_outer() as a reference : the check of indio_dev->info is performed at least twice :
* once at function start
* each time wait_event_interruptible() returns.

Quoted 2 lines

I thought it would be a good starting point.

Strictly speaking the first check there is redundant.

Quoted 10 lines

Quoted 5 lines

I think we should have one check after the poll_wait() call. If the device is unregistered we wake the pollq. So adding it there should make sure that the poll() callback is called again if the device is unregistered after the check.

No risk that an "exotic" userspace process perform 2 sys_poll() in the time between iio_device_unregister() and iio_device_free() ?
I mean : am I wrong in thinking that the filep is still valid as long as the fd has
not been released completly ?

The filep is valid as long as the file is open, same is true for the iio_dev and iio_buffer structures. The open file holds a reference to them.

What I meant with the race condition here is that we should make sure that no matter how the device unregister() and the sys_poll() are timed relative to each other it should still work as expected and not result in a indefinitely waiting poll() operation.

When multiple CPUs are involved the two operations can happen at the same time, so we need to cover that.

Quoted 13 lines

Quoted 13 lines

I'm not sure though if we'd need a explicit memory barrier or whether poll_wait() can act as a implicit one. But probably we are ok, given that poll_wait() should take a lock.

To be sure that the code is race free we need a guarantee that there is strict ordering between the indio_dev->info = NULL and wake_up() in unregister as well as between poll_wait() and the if (indio_dev->info) check in poll().

The scenario is basically

Quoted 13 lines

Quoted 8 lines

CPU0 CPU1
---------------------------------------------------------
poll_wait() indio_dev->info = NULL
if (!indio_dev->info) wake_up()

If strict ordering is not observed for either side we could end up missing the unregister and poll() will block forever just as before.

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html