Don't set sempid in semctl syscall.

From: PrasannaKumar Muralidharan <[email protected]>

As described in bug #112271 (bugzilla.kernel.org/show_bug.cgi?id=112271) don't set sempid in semctl syscall. Set sempid only when semop is called.

---
 ipc/sem.c | 1 -
 1 file changed, 1 deletion(-)

index e626965..4a99220 100644
@@ -1341,7 +1341,6 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
 		un->semadj[semnum] = 0;
 
 	curr->semval = val;
-	curr->sempid = task_tgid_vnr(current);
 	sma->sem_ctime = get_seconds();
 	/* maybe some queued-up processes were waiting for this */
 	do_smart_update(sma, NULL, 0, 0, &tasks);

2.5.0

Hi,

I disagree with the bug report:

sempid is (and always was on Linux) the pid of the last task that modified the semaphore:
It is updated for semop, SETVAL and undo adjustment on process exit.
And - that is a bug: sempid is not updated for SETALL :-(

With regard to setting sempid on SETVAL,

- Opensolaris sets sempid on SETVAL and SETALL
http://minnie.tuhs.org/cgi-bin/utree.pl?file=OpenSolaris_b135/uts/common/syscall/sem.c
- Darwin sets sempid on SETVAL and SETALL
http://fxr.watson.org/fxr/source//bsd/kern/sysv_sem.c?v=xnu-1456.1.26

Note:
For sem_otime and sem_ctime, there are also subtile differences between the sysv implementations. What should we do there?

Here is my last review (already a few years old - some links may be stale, perhaps a few more implementations are now available)
http://calculix-rpm.sourceforge.net/sysvsem.html

--
Manfred

On Fri, 26 Feb 2016, Manfred Spraul wrote:

This is a user-visible change, adding mtk.

This bug report really lacks any kind of motivation for this patch, not posix-friendly is pretty bogus. Albeit we are doing some false publicity.

Code-wise yeah, but we have such things in our docs (semctl.2):

GETPID Return the value of sempid for the semnum-th semaphore of the set (i.e., the PID of the
process that executed the last semop(2) call for the semnum-th semaphore of the set). The calling process must have read permission on the semaphore set.

Furthermore, semop.2 is very explicit about sempid. That said, I am also weary of this change because we've been setting semval for semctl for so long.
stuff).

Thanks,
Davidlohr

s/semval/sempid :)

Agreed. Is it better to change the man page and document the behaviour?

Regards,
PrasannaKumar

Requoting text I just added to the Bugzilla report to explain why the right approach seems to be to document, rather than change this behavior:

So, given that there is implementation variation that probably predates POSIX.1 (I'm assuming that the OpenSolaris behavior has an ancestry that stretches way back), I'd argue that the fault here lies with POSIX, inasmuch as it failed to capture the full variation in existing implementation behavior. (The BSD implementations of System V IPC were post facto.) Generally POSIX.1 does not try to prescribe away existing implementation behavior, but instead creates a loose spec, not that an implementation "may do such and such".

I've added the following text to the semctl(2) man page:

The sempid value
POSIX.1 defines sempid as the "process ID of [the] last opera‐ tion" on a semaphore, and explicitly notes that this value is set by a successful semop(2) call, with the implication that no other interface affects the sempid value.

While some implementations conform to the behavior specified in POSIX.1, others do not. (The fault here probably lies with POSIX.1 inasmuch as it likely failed to capture the full range of existing implementation behaviors.) Various other implemen‐ tations also update sempid for the other operations that update the value of a semaphore: the SETVAL and SETALL operations, as well as the semaphore adjustments performed on process termina‐ tion as a consequence of the use of the SEM_UNDO flag (see semop(2)).

Linux also updates sempid for SETVAL operations and semaphore adjustments. However, somewhat inconsistently, it does not update sempid for SETALL operations. While the SETALL behavior might be viewed as a bug, the behavior is longstanding, and is probably unlikely to change.

Cheers,

Michael

Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

I'm actually in favor of Linux setting sempid for SETALL, obviously as long as we don't break things. afaik there is no documentation about this, and we have a chance to do the right thing, given that we already admit to setting sempid for semctl(). Furthermore, having this exception for SETALL makes the whole situation weird and even more so ad-hoc. How about we just fix this and document it in the manpage for whatever version it lands in?

Related, it would be good to add some sort of sempid test to ltp. I've taken a look at it and there are some clear Linux-specific things being done when dealing with GETPID assuming only semop modifies the field.

Thanks,
Davidlohr

8<-----------------------------------------------------------------------
Subject: [PATCH] ipc/sem: make semctl setting sempid consistent

As indicated by bug#112271, Linux sets the sempid value upon semctl, and not only for semop calls. However, within semctl we only do this for SETVAL, leaving SETALL without updating the field, and therefore rather inconsistent behavior when compared to other Unices.

There is really no documentation regarding this and therefore users should not make assumptions. With this patch, along with updating semctl.2 manpages, this scenario should become less ambiguous As such, set sempid on SETALL cmd.

Also update some in-code documentation, specifying where the sempid is set.

---
Passes ltp and custom testcase where a child (fork) does SETALL to the set.

 ipc/sem.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

index cddd5b5..b3757ea 100644
@@ -92,7 +92,14 @@
 /* One semaphore structure for each semaphore in the system. */
 struct sem {
 	int	semval;		/* current value */
-	int	sempid;		/* pid of last operation */
+	/*
+	 * PID of the process that last modified the semaphore. For
+	 * Linux, specifically these are:
+	 *  - semop
+	 *  - semctl, via SETVAL and SETALL.
+	 *  - at task exit when performing undo adjustments (see exit_sem).
+	 */
+	int	sempid;
 	spinlock_t	lock;	/* spinlock for fine-grained semtimedop */
 	struct list_head pending_alter; /* pending single-sop operations */
 					/* that alter the semaphore */

@@ -1444,8 +1451,10 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 			goto out_unlock;
 		}
 
-		for (i = 0; i < nsems; i++)
+		for (i = 0; i < nsems; i++) {
 			sma->sem_base[i].semval = sem_io[i];
+			sma->sem_base[i].sempid = task_tgid_vnr(current);
+		}
 
 		ipc_assert_locked_object(&sma->sem_perm);
 		list_for_each_entry(un, &sma->list_id, list_id) {

2.1.4

Hi David,

Given that Linux is inconsistent both with some existing implementations (and perhaps POSIX.1) which set sempid() only on semop() and the other implementations which set sempid() on all of semop(), SETVAL, SETALL, and semadj (SEM_UNDO), there is a fair argument for modifying the current behavior.

Amen. It'd hard to know, of course, but I suspect the chances of any breakage are minute. Looking through a large body of source code (the C files in the Fedora 20 distro), there seems to be very little use of semctl(GETPID).

Yup. (Or at least not until this week :-).)

I've no objections. But, while it's difficult to see this breaking anything, it's also hard to argue compellingly for a benefit of this change. I mean: we've lived with the current behavior for 20 years, but no one seems to have minded (or perhaps we just never heard from them).

Is kselftest the better place for such tests these days? I'm not sure.

Modulo my comments above:

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/