Date: Mon, 4 Feb 2013 02:10:02 GMT From: David Xu <davidxu@freebsd.org> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead of a separate flock() call Message-ID: <201302040210.r142A2gb023261@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/175674; it has been noted by GNATS.
From: David Xu <davidxu@freebsd.org>
To: Giorgos Keramidas <keramida@FreeBSD.org>
Cc: Jukka Ukkonen <jau@iki.fi>, freebsd-gnats-submit@FreeBSD.org,
jilles@FreeBSD.org
Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead
of a separate flock() call
Date: Mon, 04 Feb 2013 10:05:31 +0800
On 2013/02/03 13:25, Giorgos Keramidas wrote:
> On 2013-01-29 18:03, Jukka Ukkonen <jau@iki.fi> wrote:
>>> Number: 175674
>>> Category: kern
>>> Synopsis: sem_open() should use O_EXLOCK with open() instead of a separate flock() call
>
>>> Environment:
>> FreeBSD sleipnir 9.1-STABLE FreeBSD 9.1-STABLE #2 r246056M: Tue Jan 29 07:33:01 EET 2013 root@sleipnir:/usr/obj/usr/src/sys/Sleipnir amd64
>>> Description:
>> sem_open() is calling flock() to set a lock on a newly created file descriptor.
>> That is pointless. The open() call a few lines before the flock() could, and
>> in my opinion should, be done with the O_EXLOCK flag set.
>
> It's also a bit safer to obtain the exclusive lock atomically before
> open() returns. Waiting for open() to complete and then calling flock()
> has a race condition.
>
> Jilles and David, do you think this patch looks ok for libc?
>
>> Patch attached with submission follows:
>>
>> --- lib/libc/gen/sem_new.c.flock 2012-11-09 18:50:05.000000000 +0200
>> +++ lib/libc/gen/sem_new.c 2012-11-09 18:44:59.000000000 +0200
>> @@ -198,11 +198,13 @@
>> goto error;
>> }
>>
>> - fd = _open(path, flags|O_RDWR|O_CLOEXEC, mode);
>> + fd = _open(path, flags|O_RDWR|O_CLOEXEC|O_EXLOCK, mode);
>> if (fd == -1)
>> goto error;
>> +#if 0
>> if (flock(fd, LOCK_EX) == -1)
>> goto error;
>> +#endif
>> if (_fstat(fd, &sb)) {
>> flock(fd, LOCK_UN);
>> goto error;
>
>
I don't think there is a race condition, the flock is used to ensure
the semaphore is already initialized, whether you use flock or E_EXLOCK,
they are same, because only one thread can lock the file at same time,
and in locked state, the code checks and initializes semaphore if it is
needed.
Regards,
David Xu
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302040210.r142A2gb023261>