The following reply was made to PR kern/175674; it has been noted by GNATS.
From: Giorgos Keramidas <keram...@freebsd.org>
To: Jilles Tjoelker <jil...@stack.nl>
Cc: Jukka Ukkonen <j...@iki.fi>, freebsd-gnats-sub...@freebsd.org,
davi...@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 <jil...@stack.nl> wrote:
> On Sun, Feb 03, 2013 at 06:25:25AM +0100, Giorgos Keramidas wrote:
> > On 2013-01-29 18:03, Jukka Ukkonen <j...@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.
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
