[PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[Manfred Spraul]

The actual Linux implementation for semctl(GETNCNT) and semctl(GETZCNT)
always (since 0.99.10) reported a thread as sleeping on all semaphores
that are listed in the semop() call.
The documented behavior (both in the Linux man page and in the Single Unix
Specification) is that a task should be reported on exactly one semaphore:
The semaphore that caused the thread to got to sleep.

This patch adds a printk_once() that is triggered if a thread hits
the relevant case.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Davidlohr Bueso <davidlohr.bueso@hp.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
 ipc/sem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/ipc/sem.c b/ipc/sem.c
index 9106321..3cc2f7b6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1000,6 +1000,18 @@ static int check_qop(struct sem_array *sma, int semnum, struct sem_queue *q,
 {
 	struct sembuf *sop = q->blocking;
 
+	/*
+	 * Linux always (since 0.99.10) reported a task as sleeping on all
+	 * semaphores. This violates SUS, therefore it was changed to the
+	 * standard compliant behavior.
+	 * Give the administrators a chance to notice that an application
+	 * might misbehave because it relies on the Linux behavior.
+	 */
+	printk_once(KERN_INFO "semctl(GETNCNT/GETZCNT) is since 3.16 Single " \
+				"Unix Specification compliant.\n" \
+				"The task %d triggered the difference, " \
+				"watch for misbehavior.", current->pid);
+
 	if (sop->sem_num != semnum)
 		return 0;
 
-- 
1.9.0

Re: [PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[Joe Perches]

On Sun, 2014-05-25 at 20:21 +0200, Manfred Spraul wrote:
> The actual Linux implementation for semctl(GETNCNT) and semctl(GETZCNT)
> always (since 0.99.10) reported a thread as sleeping on all semaphores
> that are listed in the semop() call.
> The documented behavior (both in the Linux man page and in the Single Unix
> Specification) is that a task should be reported on exactly one semaphore:
> The semaphore that caused the thread to got to sleep.
> 
> This patch adds a printk_once() that is triggered if a thread hits
> the relevant case.
[]
> diff --git a/ipc/sem.c b/ipc/sem.c
[]
> @@ -1000,6 +1000,18 @@ static int check_qop(struct sem_array *sma, int semnum, struct sem_queue *q,
>  {
>  	struct sembuf *sop = q->blocking;
>  
> +	/*
> +	 * Linux always (since 0.99.10) reported a task as sleeping on all
> +	 * semaphores. This violates SUS, therefore it was changed to the
> +	 * standard compliant behavior.
> +	 * Give the administrators a chance to notice that an application
> +	 * might misbehave because it relies on the Linux behavior.
> +	 */
> +	printk_once(KERN_INFO "semctl(GETNCNT/GETZCNT) is since 3.16 Single " \
> +				"Unix Specification compliant.\n" \
> +				"The task %d triggered the difference, " \
> +				"watch for misbehavior.", current->pid);

Unnecessary line continuations.
Missing terminating newline after "misbehavior"
Ideally coalesced or broken at linebreaks like:

	pr_info_once("semctl(GETNCNT/GETZCNT) is Single Unix Specification compliant since kernel v3.16\n"
		     "Task %d triggered the difference, watch for misbehavior\n",
		     current->pid);

>  	if (sop->sem_num != semnum)
>  		return 0;
>  

Should the printk_once (which could be pr_info_once or _ratelimited
or maybe even emitted at KERN_DEBUG) be done only when
the return is 1?

Re: [PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[Manfred Spraul]

Hi Joe,

On 05/25/2014 08:39 PM, Joe Perches wrote:
> On Sun, 2014-05-25 at 20:21 +0200, Manfred Spraul wrote:
> +	 */
> +	printk_once(KERN_INFO "semctl(GETNCNT/GETZCNT) is since 3.16 Single " \
> +				"Unix Specification compliant.\n" \
> +				"The task %d triggered the difference, " \
> +				"watch for misbehavior.", current->pid);
> Unnecessary line continuations.
> Missing terminating newline after "misbehavior"
> Ideally coalesced or broken at linebreaks like:
>
> 	pr_info_once("semctl(GETNCNT/GETZCNT) is Single Unix Specification compliant since kernel v3.16\n"
> 		     "Task %d triggered the difference, watch for misbehavior\n",
> 		     current->pid);
Thanks. I'll try to remember to really run checkpatch instead of 
assuming what it might report.
>>   	if (sop->sem_num != semnum)
>>   		return 0;
>>   
> Should the printk_once (which could be pr_info_once or _ratelimited
> or maybe even emitted at KERN_DEBUG) be done only when
> the return is 1?
>
To fully check if there is a difference would mean that the old code and 
the new code run in parallel.

The code might trigger slightly too often, but since there are zero 
known users of GETZCNT / GETNCNT simplicity wins.

--
     Manfred

Re: [PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[Davidlohr Bueso]

On Sun, 2014-05-25 at 20:21 +0200, Manfred Spraul wrote:
> The actual Linux implementation for semctl(GETNCNT) and semctl(GETZCNT)
> always (since 0.99.10) reported a thread as sleeping on all semaphores
> that are listed in the semop() call.
> The documented behavior (both in the Linux man page and in the Single Unix
> Specification) is that a task should be reported on exactly one semaphore:
> The semaphore that caused the thread to got to sleep.
> 
> This patch adds a printk_once() that is triggered if a thread hits
> the relevant case.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Cc: Davidlohr Bueso <davidlohr.bueso@hp.com>

Acked-by: Davidlohr Bueso <davidlohr@hp.com>

With some minor suggestions below.

> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> ---
>  ipc/sem.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 9106321..3cc2f7b6 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1000,6 +1000,18 @@ static int check_qop(struct sem_array *sma, int semnum, struct sem_queue *q,
>  {
>  	struct sembuf *sop = q->blocking;
>  
> +	/*
> +	 * Linux always (since 0.99.10) reported a task as sleeping on all
> +	 * semaphores. This violates SUS, therefore it was changed to the
> +	 * standard compliant behavior.
> +	 * Give the administrators a chance to notice that an application
> +	 * might misbehave because it relies on the Linux behavior.
> +	 */
> +	printk_once(KERN_INFO "semctl(GETNCNT/GETZCNT) is since 3.16 Single " \
> +				"Unix Specification compliant.\n" \
> +				"The task %d triggered the difference, " \
                                           ^^ %u (->pid is unsigned)

It would also be useful to print ->comm. I hate having to lookup the
process name with just the pid.

[PATCH 7/6] ipc/sem.c: add a printk_once for semctl(GETNCNT/GETZCNT)

[Manfred Spraul]

The actual Linux implementation for semctl(GETNCNT) and semctl(GETZCNT)
always (since 0.99.10) reported a thread as sleeping on all semaphores
that are listed in the semop() call.
The documented behavior (both in the Linux man page and in the Single Unix
Specification) is that a task should be reported on exactly one semaphore:
The semaphore that caused the thread to got to sleep.

This patch adds a pr_info_once() that is triggered if a thread hits
the relevant case.

The code triggers slightly too often, otherwise it would be necessary to
replicate the old code. As there are no known users of GETNCNT or GETZCNT,
this is done to prevent unnecessary bloat.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Davidlohr Bueso <davidlohr.bueso@hp.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Joe Perches <joe@perches.com>
---
 ipc/sem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/ipc/sem.c b/ipc/sem.c
index 9106321..3cc2f7b6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1000,6 +1000,17 @@ static int check_qop(struct sem_array *sma, int semnum, struct sem_queue *q,
 {
 	struct sembuf *sop = q->blocking;
 
+	/*
+	 * Linux always (since 0.99.10) reported a task as sleeping on all
+	 * semaphores. This violates SUS, therefore it was changed to the
+	 * standard compliant behavior.
+	 * Give the administrators a chance to notice that an application
+	 * might misbehave because it relies on the Linux behavior.
+	 */
+	pr_info_once("semctl(GETNCNT/GETZCNT) is since 3.16 Single Unix Specification compliant.\n"
+			"The task %d triggered the difference, watch for misbehavior.\n",
+			current->pid);
+
 	if (sop->sem_num != semnum)
 		return 0;
 
-- 
1.9.0