[alsa-devel] [RFC][PATCH] ALSA: control: ensure returning from blocked system-call after closing
Some operations with blocking flag for ALSA control character devices don't return to userspace when closing a file descriptor corresponding to the character device. I think this is a bug of ALSA core.
While, I find no resources to assist this aspect. In a manual of close(2)/read(2), there'no description about it. On SUSv2, there's descriptions for pipe or FIFO for this behaviour, while nothing for character devices. On Linux kernel documentation, I cannot find this kind of explaination. Thus, I just stand on my experiences to use these operations for files on filesystems, sockets and virtual character devices. It may merely be due to my misunderstanding.
Well, additionally, in unsubscribed state, read operation for the character device returns with EBADFD. On the other hand, in subscribed state and once the operations are going to be blocked, they don't return to userspace even if any unsubscribe ioctl(2) operations are executed. This losts a consistency about the actual effects of unsubscribe operation.
It's my pleasure to discuss about these bugs (and my patch candidate to fix them). You can see these bugs with this sample program.
= Sample program = #include <stdio.h> #include <stdlib.h>
#include <errno.h> #include <string.h>
#include <sys/types.h> #include <sys/stat.h> #include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h> #include <sound/asound.h>
#include <pthread.h>
static void *thread_run(void *arg) { int fd = *(int *)arg; char buf[512] = {};
while (read(fd, buf, sizeof(buf)) >= 0) ;
return (void *)NULL; }
int main(int argc, char *argv[]) { pthread_t thread = pthread_self(); int fd; int subscribe;
fd = open("/dev/snd/controlC0", O_RDONLY); if (fd < 0) { printf("%s\n", strerror(errno)); return EXIT_FAILURE; }
subscribe = 1; if (ioctl(fd, SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS, &subscribe) < 0) { printf("%s\n", strerror(errno)); close(fd); return EXIT_FAILURE; }
if (pthread_create(&thread, NULL, thread_run, (void *)&fd) < 0) { printf("%s\n", strerror(errno)); close(fd); return EXIT_FAILURE; }
sleep(1);
if (argc == 1) { close(fd); } else { subscribe = 0; if (ioctl(fd, SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS, &subscribe) < 0) { printf("%s\n", strerror(errno)); close(fd); return EXIT_FAILURE; } }
/* Here, I expect blocking state of read(2) should be released. But... */ pthread_join(thread, NULL);
return EXIT_SUCCESS; }
Takashi Sakamoto (1): ALSA: control: add .flush operation to release blocking operation
sound/core/control.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
ALSA control character devices doesn't release blocking state for read(2) operations when a file descriptor corresponding to the character device is closed by close(2). This is due to a blocking loop of snd_ctl_read() has no breaking point for this event.
Additionally, the read(2) operation reports EBADFD when executed in a state of unsubscription. On the other hand, this operation continue to be blocked after unsubscribe operation. In this case, the read(2) operation should return with EBADFD.
Furthermore, poll(2) operation for the character device can report some events after close(2).
This commit solves these issues, by adding flush file operation. The file descriptor becomes unsubscribed state after close(2) operation. Then, read(2) and poll(2) operations return with expected results.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 196a6fe..0337c16 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -113,6 +113,21 @@ static void snd_ctl_empty_read_queue(struct snd_ctl_file * ctl) spin_unlock_irqrestore(&ctl->read_lock, flags); }
+static int snd_ctl_flush(struct file *filp, fl_owner_t id) +{ + struct snd_ctl_file *cfile = filp->private_data; + + spin_lock(&cfile->read_lock); + + /* Release blocking operations. */ + cfile->subscribed = 0; + wake_up(&cfile->change_sleep); + + spin_unlock(&cfile->read_lock); + + return 0; +} + static int snd_ctl_release(struct inode *inode, struct file *file) { unsigned long flags; @@ -1380,13 +1395,16 @@ static int snd_ctl_subscribe_events(struct snd_ctl_file *file, int __user *ptr) return -EFAULT; return 0; } + + /* TODO: mutual exclusives. */ if (subscribe) { file->subscribed = 1; - return 0; } else if (file->subscribed) { snd_ctl_empty_read_queue(file); file->subscribed = 0; } + wake_up(&cfile->change_sleep); + return 0; }
@@ -1553,6 +1571,8 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer, remove_wait_queue(&ctl->change_sleep, &wait); if (ctl->card->shutdown) return -ENODEV; + if (ctl->subscribed == 0) + return -EBADFD; if (signal_pending(current)) return -ERESTARTSYS; spin_lock_irq(&ctl->read_lock); @@ -1581,17 +1601,20 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer,
static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) { - unsigned int mask; - struct snd_ctl_file *ctl; + struct snd_ctl_file *ctl = file->private_data; + unsigned int mask = 0;
- ctl = file->private_data; if (!ctl->subscribed) return 0; + poll_wait(file, &ctl->change_sleep, wait);
- mask = 0; - if (!list_empty(&ctl->events)) - mask |= POLLIN | POLLRDNORM; + spin_lock(&ctl->read_lock); + + if (!list_empty(&ctl->events) && ctl->subscribed == 1) + mask = POLLIN | POLLRDNORM; + + spin_unlock(&ctl->read_lock);
return mask; } @@ -1733,6 +1756,7 @@ static const struct file_operations snd_ctl_f_ops = .owner = THIS_MODULE, .read = snd_ctl_read, .open = snd_ctl_open, + .flush = snd_ctl_flush, .release = snd_ctl_release, .llseek = no_llseek, .poll = snd_ctl_poll,
On Sat, 25 Jul 2015 13:54:00 +0200, Takashi Sakamoto wrote:
ALSA control character devices doesn't release blocking state for read(2) operations when a file descriptor corresponding to the character device is closed by close(2). This is due to a blocking loop of snd_ctl_read() has no breaking point for this event.
Additionally, the read(2) operation reports EBADFD when executed in a state of unsubscription. On the other hand, this operation continue to be blocked after unsubscribe operation. In this case, the read(2) operation should return with EBADFD.
Furthermore, poll(2) operation for the character device can report some events after close(2).
This commit solves these issues, by adding flush file operation. The file descriptor becomes unsubscribed state after close(2) operation. Then, read(2) and poll(2) operations return with expected results.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Thanks for spotting out the problem and the patch. I guess your patch would work in most cases correctly. One thing I'm not sure is the case where the file descriptor is copied. Is the flush op called only if one of the multiple processes is closed, or is it after all fps closed? In the former case, this might be a problem.
BTW, a minor nitpick: please omit the explicit 0 or 1 comparison for subscribed field. It's a (kind of) boolean flag, so a form like if (ctl->subscribed)
or if (!ctl->subscribed)
is preferred.
thanks,
Takashi
sound/core/control.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 196a6fe..0337c16 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -113,6 +113,21 @@ static void snd_ctl_empty_read_queue(struct snd_ctl_file * ctl) spin_unlock_irqrestore(&ctl->read_lock, flags); }
+static int snd_ctl_flush(struct file *filp, fl_owner_t id) +{
- struct snd_ctl_file *cfile = filp->private_data;
- spin_lock(&cfile->read_lock);
- /* Release blocking operations. */
- cfile->subscribed = 0;
- wake_up(&cfile->change_sleep);
- spin_unlock(&cfile->read_lock);
- return 0;
+}
static int snd_ctl_release(struct inode *inode, struct file *file) { unsigned long flags; @@ -1380,13 +1395,16 @@ static int snd_ctl_subscribe_events(struct snd_ctl_file *file, int __user *ptr) return -EFAULT; return 0; }
- /* TODO: mutual exclusives. */ if (subscribe) { file->subscribed = 1;
return 0;
- wake_up(&cfile->change_sleep);
- return 0;
}
@@ -1553,6 +1571,8 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer, remove_wait_queue(&ctl->change_sleep, &wait); if (ctl->card->shutdown) return -ENODEV;
if (ctl->subscribed == 0)return -EBADFD; if (signal_pending(current)) return -ERESTARTSYS; spin_lock_irq(&ctl->read_lock);@@ -1581,17 +1601,20 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer,
static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) {
- unsigned int mask;
- struct snd_ctl_file *ctl;
- struct snd_ctl_file *ctl = file->private_data;
- unsigned int mask = 0;
- ctl = file->private_data; if (!ctl->subscribed) return 0;
- poll_wait(file, &ctl->change_sleep, wait);
- mask = 0;
- if (!list_empty(&ctl->events))
mask |= POLLIN | POLLRDNORM;
spin_lock(&ctl->read_lock);
if (!list_empty(&ctl->events) && ctl->subscribed == 1)
mask = POLLIN | POLLRDNORM;spin_unlock(&ctl->read_lock);
return mask;
} @@ -1733,6 +1756,7 @@ static const struct file_operations snd_ctl_f_ops = .owner = THIS_MODULE, .read = snd_ctl_read, .open = snd_ctl_open,
- .flush = snd_ctl_flush, .release = snd_ctl_release, .llseek = no_llseek, .poll = snd_ctl_poll,
-- 2.1.4
On 07/27/2015 12:27 PM, Takashi Iwai wrote:
On Sat, 25 Jul 2015 13:54:00 +0200, Takashi Sakamoto wrote:
ALSA control character devices doesn't release blocking state for read(2) operations when a file descriptor corresponding to the character device is closed by close(2). This is due to a blocking loop of snd_ctl_read() has no breaking point for this event.
Additionally, the read(2) operation reports EBADFD when executed in a state of unsubscription. On the other hand, this operation continue to be blocked after unsubscribe operation. In this case, the read(2) operation should return with EBADFD.
Furthermore, poll(2) operation for the character device can report some events after close(2).
This commit solves these issues, by adding flush file operation. The file descriptor becomes unsubscribed state after close(2) operation. Then, read(2) and poll(2) operations return with expected results.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Thanks for spotting out the problem and the patch. I guess your patch would work in most cases correctly.
Is it really a problem? I'd say it's the expected behavior. What close does is destroys the mapping between the process local fd and the global struct file. It does not abort any pending blocking operations. If you want to wakeup a blocking operation you need to send a signal to the thread.
One thing I'm not sure is the case where the file descriptor is copied. Is the flush op called only if one of the multiple processes is closed, or is it after all fps closed? In the former case, this might be a problem.
Should be on every close. Especially with close-on-exec this quickly becomes a problem.
Hi, Iwai-san, Lars,
Thanks for your replies. And I realize that this idea is wrong.
I wrote somewhat that blocking operations to file descriptors for files on filesystems, sockets, virtual character devices are released after closing. But this is completely wrong. I did test wrongly and had misunderstanding about these behaviour. Please drop this patch from your memory...
On Jul 28 2015 03:40, Lars-Peter Clausen wrote:
Is it really a problem? I'd say it's the expected behavior. What close does is destroys the mapping between the process local fd and the global struct file. It does not abort any pending blocking operations. If you want to wakeup a blocking operation you need to send a signal to the thread.
Indeed. Usually, blocking operations are not waken up after closing. We have only a way to do it, sending signals.
One thing I'm not sure is the case where the file descriptor is copied. Is the flush op called only if one of the multiple processes is closed, or is it after all fps closed? In the former case, this might be a problem.
Should be on every close. Especially with close-on-exec this quickly becomes a problem.
After dup()/dup2()/dup3(), file descriptors are duplicated but still refer to the same file structure. This is the same as execve() executed. These operations are completed within VFS and character device drivers or file system implementation can do nothing for it.
We can set a flag of 'close_on_exec' to file descriptors by several ways. When a file descriptsor has this flag, the old file descriptors are closed when duplicated, even if it's an operation of duplicating process image.
On Jul 27 2015 19:27, Takashi Iwai wrote:
BTW, a minor nitpick: please omit the explicit 0 or 1 comparison for subscribed field. It's a (kind of) boolean flag, so a form like if (ctl->subscribed)
or if (!ctl->subscribed)
is preferred.
OK, sure. I have a habit to write codes so that the type of variables are clear at a glance (I'm not so familiar with weak-typing language). When posting, I'll folliw to C-like idioms.
# My brain may be beaten by terrible summer heat...
Thanks
Takashi Sakamoto
participants (3)
-
Lars-Peter Clausen -
Takashi Iwai -
Takashi Sakamoto