linux-kernel - Re: [PATCH 3/3] ipc/mqueue: remove STATE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 07 Apr 2015 19:48:01 +0200
From:	Manfred Spraul <manfred@...orfullife.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	linux-kernel@...r.kernel.org
CC:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Darren Hart <darren@...art.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	fredrik.markstrom@...driver.com, Davidlohr Bueso <davidlohr@...com>
Subject: Re: [PATCH 3/3] ipc/mqueue: remove STATE_PENDING

On 04/07/2015 05:03 PM, Sebastian Andrzej Siewior wrote:
> This patch moves the wakeup_process() invocation so it is not done under
> the info->lock. With this change, the waiter is woken up once it is
> "ready" which means its state is STATE_READY and it does not need to loop
> on SMP if it is still in STATE_PENDING.
> In the timeout case we still need to grab the info->lock to verify the state.
>
> This change should also avoid the introduction of preempt_disable() in
> -RT which avoids a busy-loop which pools for the STATE_PENDING -> STATE_READY
> change if the waiter has a higher priority compared to the waker.

> @@ -909,9 +905,8 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
>    * bypasses the message array and directly hands the message over to the
>    * receiver.
>    * The receiver accepts the message and returns without grabbing the queue
> - * spinlock. Therefore an intermediate STATE_PENDING state and memory barriers
> - * are necessary. The same algorithm is used for sysv semaphores, see
> - * ipc/sem.c for more details.
> + * spinlock. The same algorithm is used for sysv semaphores, see ipc/sem.c
> + * for more details.
No. With your change, ipc/sem.c and ipc/msg.c use different algorithms.
Please update the comment and describe the new approach:

Current approach:
- set pointer to message
- STATE_PENDING
- wake_up_process()
- STATE_READY
     (now the receiver can continue)

New approach:
- set pointer to message
- get_task_struct
- STATE_READY
     (now the receiver can continue, e.g. woken up due to an unrelated 
SIGKILL)
- wake_up_process()
- put_task_struct()


> +		if (r_sender) {
> +			wake_up_process(r_sender);
> +			put_task_struct(r_sender);
> +		}
>   		ret = 0;
Could you double-check that it is safe to call wake_up_process on a 
killed and reaped thread, only with a get_task_struct reference?

And: please test it, too. (patch the kernel so that you can trigger this 
case).

--
     Manfred

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/