Date: Sun, 3 Feb 2013 21:00:01 GMT From: Giorgos Keramidas <keramida@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: <201302032100.r13L01PG044439@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: Giorgos Keramidas <keramida@FreeBSD.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: Jukka Ukkonen <jau@iki.fi>, freebsd-gnats-submit@FreeBSD.org, davidxu@FreeBSD.org Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead of a separate flock() call Date: Sun, 3 Feb 2013 21:49:55 +0100 On 2013-02-03 21:20, Jilles Tjoelker <jilles@stack.nl> wrote: > On Sun, Feb 03, 2013 at 06:25:25AM +0100, 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; > > For a reason unknown to me, open(2) does not restart but always > returns [EINTR] when a signal is caught. This is not POSIX-compliant. > On the other hand, flock(2) is not broken in this way. So this change > breaks sem_open(3) in the unlikely case that a signal with SA_RESTART > arrives while it is waiting for the lock. I see where kern_openat() returns an error when vn_open is interrupted: 1083 error = vn_open(&nd, &flags, cmode, fp); 1084 if (error) { .... 1109 if (error == ERESTART) 1110 error = EINTR; 1111 goto bad; 1112 } > The best way to fix this is in kern_openat() in the kernel but this > might cause compatibility issues. Not sure if there would be serious compatibility problems if open() would automatically restart instead of returning EINTR. It definitely seems a rather intrusive change though. > The #if 0 is silly; we have version control to restore old code if > need be. That's true. I'm guessing the OP wanted to keep this around for testing purposes. We don't really need the old code in an #if 0 block.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302032100.r13L01PG044439>