Bug 206927 - Bad semaphore return code on collision
Summary: Bad semaphore return code on collision
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.2-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: Jilles Tjoelker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-04 18:38 UTC by karl
Modified: 2016-04-09 13:34 UTC (History)
1 user (show)

See Also:
jilles: mfc-stable10+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description karl 2016-02-04 18:38:30 UTC
Attempting to upgrade Postgres from 9.4.5 to 9.5.0 I ran into the following error:

$ initdb -D data-default
...
creating template1 database in data-default/base/1 ... FATAL:  could not
create semaphores: Invalid argument
DETAIL:  Failed system call was semget(2, 17, 03600).

This appears to be an incorrect return; semaphore key "2" is indeed in use but by a different, unrelated process (the web server.)


This is what Tom Lane from Postgres had to say about their code, which appears to make sense:

******************** from postgres mailing list ***************

Hmm.  On my Linux box, "man semget" says EINVAL means

       EINVAL nsems  is less than 0 or greater than the limit on the number of
              semaphores per semaphore set (SEMMSL), or a semaphore set corre-
              sponding  to  key  already  exists, and nsems is larger than the
              number of semaphores in that set.

which agrees with the POSIX spec.  Is FreeBSD the same?

Proceeding on the assumption that it is ...

17 is the same nsems value we've been using for donkey's years, so the
SEMMSL aspect of this seems unlikely to apply; what presumably is
happening is a collision with an existing semaphore's key.  Our code is
prepared for that, but it expects a different error code in such cases,
either EEXIST or EACCES:

        /*
         * Fail quietly if error indicates a collision with existing set. One
         * would expect EEXIST, given that we said IPC_EXCL, but perhaps we
         * could get a permission violation instead?  Also, EIDRM might occur
         * if an old set is slated for destruction but not gone yet.
         */

It sounds like your kernel is returning EINVAL in preference to any of
those codes, which would be pretty broken.  I do not want to make our code
treat EINVAL as meaning we should retry with a different key, because if
the problem is indeed the SEMMSL limit, we'd be in an infinite loop.

You can probably get past this for the moment if you can remove the
semaphore set with key 2, but I'd advise filing a FreeBSD kernel
bug about their choice of errno.

********** end ***********

Indeed our (FreeBSD) man page says:

     The semget() system call will fail if:

     [EACCES]           Access permission failure.

     [EEXIST]           IPC_CREAT and IPC_EXCL were specified, and a semaphore
                        set corresponding to key already exists.

     [EINVAL]           The number of semaphores requested exceeds the system
                        imposed maximum per set.

     [ENOSPC]           Insufficiently many semaphores are available.

     [ENOSPC]           The kernel could not allocate a struct semid_ds.

     [ENOENT]           No semaphore set was found corresponding to key, and
                        IPC_CREAT was not specified.

EINVAL is an incorrect return under this circumstance; EEXIST appears to be the expected returned error code.
Comment 1 Jilles Tjoelker freebsd_committer freebsd_triage 2016-02-07 21:22:29 UTC
The additional [EINVAL] condition is missing from the semget(2) man page; I will add it.

Although POSIX leaves the detection order of errors undefined (XSH 2.3 Error Numbers), detecting [EINVAL] before [EEXIST] is clearly inconvenient for applications. The inconvenient order has been in place for years, but it is easy to change.

You could request the Austin Group to make this unambiguous (e.g., restrict part of the [EINVAL] error to not occur if ((semflg&IPC_CREAT)&&(segflg&IPC_EXCL)) is non-zero).
Comment 2 commit-hook freebsd_committer freebsd_triage 2016-02-07 21:25:16 UTC
A commit references this bug:

Author: jilles
Date: Sun Feb  7 21:25:08 UTC 2016
New revision: 295384
URL: https://svnweb.freebsd.org/changeset/base/295384

Log:
  semget(2): Add missing [EINVAL] conditions.

  PR:		206927

Changes:
  head/lib/libc/sys/semget.2
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-02-07 22:13:21 UTC
A commit references this bug:

Author: jilles
Date: Sun Feb  7 22:12:39 UTC 2016
New revision: 295385
URL: https://svnweb.freebsd.org/changeset/base/295385

Log:
  semget(): Check for [EEXIST] error first.

  Although POSIX literally permits failing with [EINVAL] if IPC_CREAT and
  IPC_EXCL were both passed, the semaphore set already exists and has fewer
  semaphores than nsems, this does not allow an application to retry safely:
  if the [EINVAL] is actually because of the semmsl limit, an infinite loop
  would result.

  PR:		206927

Changes:
  head/sys/kern/sysv_sem.c
  head/tools/regression/sysvsem/semtest.c
Comment 4 Jilles Tjoelker freebsd_committer freebsd_triage 2016-02-21 21:57:15 UTC
Fixed in head two weeks ago. I plan to MFC to stable/10 after 10.3-RELEASE.
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-04-08 15:44:04 UTC
A commit references this bug:

Author: jilles
Date: Fri Apr  8 15:43:49 UTC 2016
New revision: 297719
URL: https://svnweb.freebsd.org/changeset/base/297719

Log:
  MFC r295384: semget(2): Add missing [EINVAL] conditions.

  PR:		206927

Changes:
_U  stable/10/
  stable/10/lib/libc/sys/semget.2
Comment 6 commit-hook freebsd_committer freebsd_triage 2016-04-09 13:32:51 UTC
A commit references this bug:

Author: jilles
Date: Sat Apr  9 13:32:42 UTC 2016
New revision: 297747
URL: https://svnweb.freebsd.org/changeset/base/297747

Log:
  MFC r295385: semget(): Check for [EEXIST] error first.

  Although POSIX literally permits failing with [EINVAL] if IPC_CREAT and
  IPC_EXCL were both passed, the semaphore set already exists and has fewer
  semaphores than nsems, this does not allow an application to retry safely:
  if the [EINVAL] is actually because of the semmsl limit, an infinite loop
  would result.

  PR:		206927

Changes:
_U  stable/10/
  stable/10/sys/kern/sysv_sem.c
  stable/10/tools/regression/sysvsem/semtest.c