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@xxxxxxxxx>

Cheers,

Michael

> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
> ---
> 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/