[PATCH] alsactl: Skip restore during the lock

[PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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;
 }
-- 
2.26.2

Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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

Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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

Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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

Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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

Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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.

Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

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