[PATCH] Don't set sempid in semctl syscall.

[PATCH] Don't set sempid in semctl syscall.

[PrasannaKumar Muralidharan]

From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

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.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
 ipc/sem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index e626965..4a99220 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -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

Re: [PATCH] Don't set sempid in semctl syscall.

[Manfred Spraul]

Hi,

On 02/26/2016 01:21 PM, PrasannaKumar Muralidharan wrote:
> From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>
> 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.
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

Re: [PATCH] Don't set sempid in semctl syscall.

[Davidlohr Bueso]

On Fri, 26 Feb 2016, Manfred Spraul wrote:

This is a user-visible change, adding mtk.

>Hi,
>
>On 02/26/2016 01:21 PM, PrasannaKumar Muralidharan wrote:
>>From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>>
>>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.
>I disagree with the bug report:

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.

>
>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 :-(

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

Re: [PATCH] Don't set sempid in semctl syscall.

[Davidlohr Bueso]

On Fri, 26 Feb 2016, Davidlohr Bueso wrote:

>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.

s/semval/sempid :)

Re: [PATCH] Don't set sempid in semctl syscall.

[PrasannaKumar Muralidharan]

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

Regards,
PrasannaKumar

Re: [PATCH] Don't set sempid in semctl syscall.

[Michael Kerrisk]

On Sat, Feb 27, 2016 at 9:42 AM, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> Agreed. Is it better to change the man page and document the behaviour?

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/

Re: [PATCH] Don't set sempid in semctl syscall.

[Davidlohr Bueso]

On Sun, 28 Feb 2016, Michael Kerrisk wrote:

>       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.

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.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
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(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index cddd5b5..b3757ea 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -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

Re: [PATCH] Don't set sempid in semctl syscall.

[Michael Kerrisk (man-pages)]

Hi David,

On 02/29/2016 10:22 PM, Davidlohr Bueso wrote:
> On Sun, 28 Feb 2016, Michael Kerrisk wrote:
> 
>>       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.
> 
> I'm actually in favor of Linux setting sempid for SETALL, 

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.

> obviously as
> long as we don't break things. 

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).

> afaik there is no documentation about this,

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

> 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?

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).

> 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.

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

> 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.

Modulo my comments above:

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

Cheers,

Michael

> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> 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(-)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index cddd5b5..b3757ea 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -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) {
> 


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