[PATCH] alsactl: Skip restore during the lock
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1179904 Signed-off-by: Takashi Iwai tiwai@suse.de --- alsactl/state.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/alsactl/state.c b/alsactl/state.c index ea1d3bcaaddf..8d1d814341bd 100644 --- a/alsactl/state.c +++ b/alsactl/state.c @@ -1663,7 +1663,12 @@ int load_state(const char *file, const char *initfile, int initflags, err = snd_input_stdio_attach(&in, stdin, 0); } else { lock_fd = state_lock(file, 10); - err = lock_fd >= 0 ? snd_input_stdio_open(&in, file, "r") : lock_fd; + /* skip restore if already locked; i.e. concurrent accesses */ + if (lock_fd < 0) { + err = 0; + goto out_global; + } + err = snd_input_stdio_open(&in, file, "r"); } if (err >= 0) { err = snd_config_load(config, in); @@ -1781,6 +1786,7 @@ single: err = finalerr; out: snd_config_delete(config); +out_global: snd_config_update_free_global(); return err; }
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Jaroslav
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
thanks,
Takashi
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--; + } if (errno == EEXIST){ + /* race at creating a lock, try again */ + continue; } else { err = -errno; goto out;
Takashi
Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--;
} if (errno == EEXIST){/* race at creating a lock, try again */continue; } else { err = -errno; goto out;
If we don't use the sleep call and the timeout counter, there's endless CPU busy loop when the root creates the lock file and user wants to access it for example. It's better to add EEXIST to the previous errno condition.
Jaroslav
On Fri, 11 Dec 2020 18:23:03 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--;
} if (errno == EEXIST){/* race at creating a lock, try again */continue; } else { err = -errno; goto out;If we don't use the sleep call and the timeout counter, there's endless CPU busy loop when the root creates the lock file and user wants to access it for example. It's better to add EEXIST to the previous errno condition.
The timeout is decreased in the while condition.
Takashi
Dne 11. 12. 20 v 18:37 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 18:23:03 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--;
} if (errno == EEXIST){/* race at creating a lock, try again */continue; } else { err = -errno; goto out;If we don't use the sleep call and the timeout counter, there's endless CPU busy loop when the root creates the lock file and user wants to access it for example. It's better to add EEXIST to the previous errno condition.
The timeout is decreased in the while condition.
It seems not correct. It decreases the wait time to half. My fault :-(
What about this straight change:
--- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -63,11 +63,15 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (fd < 0) { if (errno == EBUSY || errno == EAGAIN) { sleep(1); - timeout--; - } else { - err = -errno; - goto out; + continue; } + if (errno == EEXIST) { + fd = open(nfile, O_RDWR); + if (fd >= 0) + break; + } + err = -errno; + goto out; } } } Jaroslav
On Fri, 11 Dec 2020 18:56:56 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:37 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 18:23:03 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a): > Currently alsactl-restore tries to initialize the device when an error > is found for restore action. But this isn't the right behavior in the > case where the lock is held; it implies that another alsactl is > running concurrently, hence you shouldn't initialize the card at the > same time. The situation is found easily when two alsactls get > started by both udev and systemd (note that those two invocations are > the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules > for details). > > This patch changes load_state() not to handle the initialization if > the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--;
} if (errno == EEXIST){/* race at creating a lock, try again */continue; } else { err = -errno; goto out;If we don't use the sleep call and the timeout counter, there's endless CPU busy loop when the root creates the lock file and user wants to access it for example. It's better to add EEXIST to the previous errno condition.
The timeout is decreased in the while condition.
It seems not correct. It decreases the wait time to half. My fault :-(
What about this straight change:
--- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -63,11 +63,15 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (fd < 0) { if (errno == EBUSY || errno == EAGAIN) { sleep(1);
timeout--;} else {err = -errno;goto out;
continue; }if (errno == EEXIST) {fd = open(nfile, O_RDWR);if (fd >= 0)break;}err = -errno;goto out; } } }
Yes, this should work. Shall I resubmit? I'd split to two, one to correct doubly timeout decreases and another to handle EEXIST.
thanks,
Takashi
Dne 11. 12. 20 v 19:44 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 18:56:56 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:37 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 18:23:03 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:59:05 +0100, Takashi Iwai wrote:
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote: > > Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a): >> Currently alsactl-restore tries to initialize the device when an error >> is found for restore action. But this isn't the right behavior in the >> case where the lock is held; it implies that another alsactl is >> running concurrently, hence you shouldn't initialize the card at the >> same time. The situation is found easily when two alsactls get >> started by both udev and systemd (note that those two invocations are >> the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules >> for details). >> >> This patch changes load_state() not to handle the initialization if >> the locking fails. > > The operation should serialize in this case (there's limit of 10 seconds which > should be enough to finish the initialization). The state_lock() function > should return -EBUSY when the file is locked (and I'm fine to change the > behaviour from 'init' to 'skip' for this lock state). > > It seems that -EEXIST is returned when the lock file exists, but the > open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access > this file when another user owns the file. > > But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and > /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
... something like below
diff --git a/alsactl/lock.c b/alsactl/lock.c index 4a485392b3bd..c1c30f0c5eee 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (errno == EBUSY || errno == EAGAIN) { sleep(1); timeout--;
} if (errno == EEXIST){/* race at creating a lock, try again */continue; } else { err = -errno; goto out;If we don't use the sleep call and the timeout counter, there's endless CPU busy loop when the root creates the lock file and user wants to access it for example. It's better to add EEXIST to the previous errno condition.
The timeout is decreased in the while condition.
It seems not correct. It decreases the wait time to half. My fault :-(
What about this straight change:
--- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -63,11 +63,15 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd) if (fd < 0) { if (errno == EBUSY || errno == EAGAIN) { sleep(1);
timeout--;} else {err = -errno;goto out;
continue; }if (errno == EEXIST) {fd = open(nfile, O_RDWR);if (fd >= 0)break;}err = -errno;goto out; } } }Yes, this should work. Shall I resubmit? I'd split to two, one to correct doubly timeout decreases and another to handle EEXIST.
Yes, thanks.
Jaroslav
Dne 11. 12. 20 v 17:59 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
Maybe. It seems enough to add EEXIST errno check to the "if (errno == EBUSY || errno == EAGAIN)" condition to repeat the open sequence. The -EBUSY will be returned correctly then. The one second delay is harmless in my eyes for the second task.
Jaroslav
-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Fri, 11 Dec 2020 18:20:50 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 17:59 Takashi Iwai napsal(a):
On Fri, 11 Dec 2020 17:45:45 +0100, Jaroslav Kysela wrote:
Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
Currently alsactl-restore tries to initialize the device when an error is found for restore action. But this isn't the right behavior in the case where the lock is held; it implies that another alsactl is running concurrently, hence you shouldn't initialize the card at the same time. The situation is found easily when two alsactls get started by both udev and systemd (note that those two invocations are the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules for details).
This patch changes load_state() not to handle the initialization if the locking fails.
The operation should serialize in this case (there's limit of 10 seconds which should be enough to finish the initialization). The state_lock() function should return -EBUSY when the file is locked (and I'm fine to change the behaviour from 'init' to 'skip' for this lock state).
It seems that -EEXIST is returned when the lock file exists, but the open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access this file when another user owns the file.
But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and /lib/systemd/system/alsa-restore.service should be run as root, right?
Yes, it should be root.
I also wondered how EEXIST comes, too. Maybe it's also the race between the first open(O_RDWR) and the second open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again to the normal open(O_RDWR)?
Maybe. It seems enough to add EEXIST errno check to the "if (errno == EBUSY || errno == EAGAIN)" condition to repeat the open sequence. The -EBUSY will be returned correctly then. The one second delay is harmless in my eyes for the second task.
I'm afraid that a significant delay can be confusing. And this should be the race only once, so no need to add the artificial delay, IMO.
(BTW, now I noticed that we decrease timeout twice :)
Takashi
participants (2)
-
Jaroslav Kysela -
Takashi Iwai