iio:buffer: polling unregistered device stalls user process

PATCH Superseded Gregor Boirie 4 participants 8 messages Sep 7, 2016 – Sep 8, 2016 IIO

Share & tools

Gregor Boirie

author

2016-09-07 17:22 UTC #0 lore raw

cover iio:buffer: polling unregistered device stalls user process

Unregistering a device currently in use may stall userspace process polling for data availability (poll syscall). Here is a typical situation detailed below.

zpa2326 barometer driver is loaded.
<console>
/ # lsmod

Module                  Size  Used by
zpa2326_i2c             2833  0
zpa2326                25360  1 zpa2326_i2c

[...]
</console>

zpa2326 has registered iio device 0 and its own hardware trigger 0. <console>
/ # lsiio
Device 000: zpa2326
Trigger 000: zpa2326-dev0
/ # ls /dev
bus ptmx tty29 tty52
[...]
iio:device0 tty11 tty35 tty59
</console>

Run an everlasting sampling process (iio_generic_buffer) in hardware triggered buffer mode then unload module (zpa2326_i2c) driving the device (iio:device0) under use.
<console>
/ # iio_generic_buffer -a -N 0 -l 2 -c -1 & sleep 1 ; rmmod zpa2326_i2c iio device number being used is 0
iio trigger number being used is 0
No channels are enabled, enabling all channels
Enabling: in_pressure_en
Enabling: in_timestamp_en
Enabling: in_temp_en
/sys/bus/iio/devices/iio:device0 zpa2326-dev0
103.736877 31265.359375 98504890900
103.734001 31369.199219 98568173608
103.739883 31369.199219 98631470327
103.734070 31369.199219 98694748816
103.733070 31369.199219 98758036681
103.736130 31473.039062 98821330691
103.739006 31369.199219 98884620014
103.737068 31473.039062 98947911524
103.732254 31473.039062 99011214754
103.739006 31473.039062 99074498920
103.739006 31473.039062 99137784962
103.734192 31473.039062 99201085431
103.737190 31473.039062 99264364910
103.741066 31473.039062 99327655274
/ #
</console>

After 1 second, iio_generic_buffer output has been muted. A ps shows iio_generic_buffer is still there, in interruptible sleep state. <console>
/ # ps | grep iio_generic_buffer

  123 0         1820 S    iio_generic_buffer -a -N 0 -l 2 -c -1
  128 0         2776 S    grep iio_generic_buffer

/ #
</console>

Strace shows iio_generic_buffer is stalled into poll syscall. Forever... <console>
/ # strace -p 123
Process 123 attached
poll([{fd=3, events=POLLIN}], 1, -1^CProcess 123 detached
<detached ...>
</console>

A sysrq task state trace shows iio_generic_buffer is blocking in... <console>
/ # [ 202.859427] sysrq: SysRq : Show State

[  202.863151]   task                PC stack   pid father
[...]
[  205.667101] iio_generic_buf S 8068a254     0   123     98 0x00000000
[  205.673507] [<8068a254>] (__schedule) from [<8068ac0c>] (schedule+0x64/0x100)
[  205.680664] [<8068ac0c>] (schedule) from [<80690820>] (schedule_hrtimeout_range_clock+0x1d8/0x1e8)
[  205.689641] [<80690820>] (schedule_hrtimeout_range_clock) from [<80690858>] (schedule_hrtimeout_range+0x28/0x30)
[  205.699831] [<80690858>] (schedule_hrtimeout_range) from [<8022b148>] (poll_schedule_timeout+0x84/0xc8)
[  205.709241] [<8022b148>] (poll_schedule_timeout) from [<8022d808>] (do_sys_poll+0x640/0x8c4)
[  205.717697] [<8022d808>] (do_sys_poll) from [<8022db0c>] (do_restart_poll+0x80/0xd4)
[  205.725461] [<8022db0c>] (do_restart_poll) from [<8004f660>] (sys_restart_syscall+0x30/0x34)
[  205.733918] [<8004f660>] (sys_restart_syscall) from [<800151a0>] (ret_fast_syscall+0x0/0x1c)
[  205.742351] Showing busy workqueues and worker pools:
[  205.747406] workqueue events: flags=0x0
[  205.751243]   pwq 4: cpus=2 node=0 flags=0x0 nice=0 active=1/256
[  205.757304]     in-flight: 23:check_carrier
[  205.761551] pool 4: cpus=2 node=0 flags=0x0 nice=0 hung=0s workers=4 idle: 61 77 65

</console>

Killing iio_generic_buffer works.
<console>
/ # kill 123
/ # Caught signal 15
failed to open /sys/bus/iio/devices/iio:device0/buffer/enable
Failed to disable buffer: No such file or directory
Enabling/disabling channels: can't open /sys/bus/iio/devices/iio:device0/scan_elements Failed to disable all channels
[1]+ Done(241) iio_generic_buffer -a -N 0 -l 2 -c -1
/ #
</console>

This situation happens when the device is being unregistered (iio_device_unregister()) while a userspace process is executing the poll syscall.
If device has vanished before running the iio_buffer_fileops poll hook, the latter will keep returning no events and process will be stalled waiting for events that will never come if no timeout has been specified.

This patch fixes this by ensuring iio_buffer_poll() returns POLLERR if device has been previously unregistered. Userspace process will be properly notified something wrong happened so that it may take appropriate action.

Running the same test as above will now show.
<console>
/ # iio_generic_buffer -a -N 0 -l 2 -c -1 & sleep 1; rmmod zpa2326_i2c iio device number being used is 0
iio trigger number being used is 0
No channels are enabled, enabling all channels
Enabling: in_pressure_en
Enabling: in_timestamp_en
Enabling: in_temp_en
/sys/bus/iio/devices/iio:device0 zpa2326-dev0
103.679695 31680.718750 54318428312
103.684631 31680.718750 54381713521
103.682693 31680.718750 54444999458
103.681694 31784.558594 54508288521
103.679878 31784.558594 54571584458
103.680756 31784.558594 54634876698
103.677002 31784.558594 54698161020
103.682755 31784.558594 54761463208
103.681816 31784.558594 54824749979
103.673004 31784.558594 54888037583
103.675819 31784.558594 54951331958
103.679878 31784.558594 55014612270
103.671066 31784.558594 55077905447
103.683693 31784.558594 55141192583
failed to open /sys/bus/iio/devices/iio:device0/buffer/enable
Failed to disable buffer: No such file or directory
Enabling/disabling channels: can't open /sys/bus/iio/devices/iio:device0/scan_elements Failed to disable all channels
[1]+ Done(1) iio_generic_buffer -a -N 0 -l 2 -c -1
</console>

Regards,
Gregor.

Gregor Boirie (2):
iio:buffer: properly handle polling of unregistered device iio:buffer: document iio_buffer_poll() return value

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

Signature · 1 line

2.1.4

Gregor Boirie

author

2016-09-07 17:26 UTC #1 lore raw

PATCH 1/2 iio:buffer: properly handle polling of unregistered device

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).

Fixes 1bdc029390 ("iio: industrialio-buffer: Fix iio_buffer_poll return value")

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

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

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

index 90462fc..b15b3386 100644
@@ -162,13 +162,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;
 }

Signature · 1 line

2.1.4

Lars-Peter Clausen

2016-09-07 18:13 UTC #2 lore raw

Re: [PATCH v1 1/2] iio:buffer: properly handle polling of unregistered device

Quoted 7 lines · Gregor Boirie

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).

Quoted 6 lines

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).
Fixes 1bdc029390 ("iio: industrialio-buffer: Fix iio_buffer_poll return value")
Signed-off-by Gregor Boirie <[email protected]>

Hi,

Thanks for the patch. I agree with the solution, I don't quite agree with the description. In my opinion the current behavior is not necessarily a bug. Obviously there won't be a wakeup when the device has been removed since no new data will be generated. But there are other scenarios where this can happen as well (e.g. broken hardware). An application should be prepared to handle this and if it is interactive for example allow the user to interrupt the capture process.

Nevertheless adding support for returning an error on poll() when the device has been removed is a good idea.

Quoted 9 lines

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

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

index 90462fc..b15b3386 100644

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 +162,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.

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

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;

Why check indio_dev->info twice though? I think we should check it only once 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.

Quoted 8 lines

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

+
+	if (rdy)
 		return POLLIN | POLLRDNORM;
+
 	return 0;
 }

Gregor Boirie

author

2016-09-08 08:11 UTC #3 lore raw

Re: [PATCH v1 1/2] iio:buffer: properly handle polling of unregistered device

Hi Lars,

Quoted 16 lines · Lars-Peter Clausen

Quoted 7 lines · Gregor Boirie
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).
Quoted 6 lines
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).
Fixes 1bdc029390 ("iio: industrialio-buffer: Fix iio_buffer_poll return value")
Signed-off-by Gregor Boirie <[email protected]>
Hi,

Quoted 7 lines

Thanks for the patch. I agree with the solution, I don't quite agree with the description. In my opinion the current behavior is not necessarily a bug. Obviously there won't be a wakeup when the device has been removed since no new data will be generated. But there are other scenarios where this can happen as well (e.g. broken hardware). An application should be prepared to handle this and if it is interactive for example allow the user to interrupt the capture process.

Ok. What do you want me to do then ? Remove the "Fixes" tag ? Reword something in the commit log ?
I'll merge the 2 patches as recommended by Jonathan anyway.

Quoted 44 lines

Nevertheless adding support for returning an error on poll() when the device has been removed is a good idea.
Quoted 9 lines
---
  drivers/iio/industrialio-buffer.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)
drivers/iio/industrialio-buffer.c +0 -0
index 90462fc..b15b3386 100644
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 +162,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.
drivers/iio/industrialio-buffer.c unsigned int iio_buffer_poll(struct file *filp, · line 174
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;
Why check indio_dev->info twice though? I think we should check it only once 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.
Quoted 8 lines
drivers/iio/industrialio-buffer.c +3 -0
+
+	if (rdy)
  		return POLLIN | POLLRDNORM;
+
  	return 0;
  }
 
 

Lars-Peter Clausen

2016-09-08 16:14 UTC #4 lore raw

Re: [PATCH v1 1/2] iio:buffer: properly handle polling of unregistered device

Quoted 27 lines · Gregor Boirie

Hi Lars,
Quoted 17 lines · Lars-Peter Clausen
Quoted 7 lines · Gregor Boirie
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).
Quoted 7 lines
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).
Fixes 1bdc029390 ("iio: industrialio-buffer: Fix iio_buffer_poll return
value")
Signed-off-by Gregor Boirie <[email protected]>
Hi,
Quoted 7 lines
Thanks for the patch. I agree with the solution, I don't quite agree with the description. In my opinion the current behavior is not necessarily a bug. Obviously there won't be a wakeup when the device has been removed since no new data will be generated. But there are other scenarios where this can happen as well (e.g. broken hardware). An application should be prepared to handle this and if it is interactive for example allow the user to interrupt the capture process.

Quoted 3 lines

Ok. What do you want me to do then ? Remove the "Fixes" tag ? Reword something in the commit log ?
I'll merge the 2 patches as recommended by Jonathan anyway.

I'd drop the Fixes tag and maybe change the subject to "Let poll report an error for unregistered devices"

Jonathan Cameron

2016-09-08 17:21 UTC #5 lore raw

Re: [PATCH v1 1/2] iio:buffer: properly handle polling of unregistered device

Quoted 39 lines · Lars-Peter Clausen

Quoted 5 lines · Gregor Boirie
Hi Lars,
Quoted 2 lines · Lars-Peter Clausen
Quoted 1 line · Gregor Boirie
Using iio_device_unregister() on a device currently in use may
stall
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
userspace process polling for data availability (poll syscall).
If device has vanished before running the iio_buffer_fileops poll
hook, the
Quoted 1 line
Quoted 1 line
Quoted 1 line
latter will return empty poll event mask. Process will be stalled
waiting
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
for events that will never come (if no timeout specified).
This patch ensures iio_buffer_poll() returns POLLERR if device has
just
Quoted 1 line
Quoted 1 line
Quoted 1 line
been unregistered in order to properly notify userspace process
something
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
wrong happened (such as removable device unplugged).
Fixes 1bdc029390 ("iio: industrialio-buffer: Fix iio_buffer_poll
return
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
value")
Signed-off-by Gregor Boirie <[email protected]>
Hi,
Thanks for the patch. I agree with the solution, I don't quite agree
with
Quoted 1 line
Quoted 1 line
the description. In my opinion the current behavior is not
necessarily a
Quoted 1 line
Quoted 1 line
bug. Obviously there won't be a wakeup when the device has been
removed
Quoted 1 line
Quoted 1 line
since no new data will be generated. But there are other scenarios
where
Quoted 1 line
Quoted 1 line
this can happen as well (e.g. broken hardware). An application
should be
Quoted 1 line
Quoted 1 line
prepared to handle this and if it is interactive for example allow
the user

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
to interrupt the capture process.
Ok. What do you want me to do then ? Remove the "Fixes" tag ? Reword something in the commit log ?
I'll merge the 2 patches as recommended by Jonathan anyway.
I'd drop the Fixes tag and maybe change the subject to "Let poll report an
error for unregistered devices"

Agreed.

I guess this is in the no one should notice from the ABI change point of view.

Quoted 5 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

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Gregor Boirie

author

2016-09-07 17:26 UTC #6 lore raw

PATCH 2/2 iio:buffer: document iio_buffer_poll() return value

In reply to Gregor Boirie

Add documentation for iio_buffer_poll() POLLERR return value.

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

---
 drivers/iio/industrialio-buffer.c | 1 +
 1 file changed, 1 insertion(+)

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

index b15b3386..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,

Signature · 1 line

2.1.4

Jonathan Cameron

maintainer

2016-09-07 19:06 UTC #7 lore raw

Re: [PATCH v1 2/2] iio:buffer: document iio_buffer_poll() return value

Quoted 3 lines · Gregor Boirie

Add documentation for iio_buffer_poll() POLLERR return value.
Signed-off-by Gregor Boirie <[email protected]>

I'd roll this into the previous patch. No need for a separate one to document the change.

Jonathan

Quoted 17 lines

---
 drivers/iio/industrialio-buffer.c | 1 +
 1 file changed, 1 insertion(+)

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

index b15b3386..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,