linux-kernel - Re: [RFC PATCH] ipc/mqueue: add detector of wakeup race

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>] [day] [month] [year] [list]

Date:   Fri, 14 May 2021 17:38:30 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Hillf Danton <hdanton@...a.com>, Davidlohr Bueso <dbueso@...e.de>
Cc:     Matthias von Faber <matthias.vonfaber@...-tech.de>,
        Varad Gautam <varad.gautam@...e.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] ipc/mqueue: add detector of wakeup race

Hi Hillf,

On 5/11/21 9:36 AM, Hillf Danton wrote:
> Apart from the pipeline wakeup, there are wakeups from timer timeout, signal,
> and spurious sources. Because the race between pipeline and non pipeline
> wakeups is not frequent, we handle it under lock on the waiter side.
>
> On the waker side, wait condition is set true before adding waiter to the wake
> queue to cut the chance for missing wakup, and it is updated to be READY after
> wake_q_add to show that the race window is closed by the waker and it is safe
> now for waiter to go home in bid to cut the risk that waiter exits while waker
> is dereferencing waiter's resouces.
>
> Based on the initial work from Varad Gautam.
>
> Signed-off-by: Hillf Danton <hdanton@...a.com>
> ---
>
> --- y/ipc/mqueue.c
> +++ x/ipc/mqueue.c
> @@ -55,7 +55,8 @@ struct mqueue_fs_context {
>   #define RECV		1
>   
>   #define STATE_NONE	0
> -#define STATE_READY	1
> +#define STATE_TRUE	1
> +#define STATE_READY	2 /* detector of wakeup race */
>   
>   struct posix_msg_tree_node {
>   	struct rb_node		rb_node;
> @@ -722,7 +723,7 @@ static int wq_sleep(struct mqueue_inode_
>   		spin_lock(&info->lock);
>   
>   		/* we hold info->lock, so no memory barrier required */
> -		if (READ_ONCE(ewp->state) == STATE_READY) {
> +		if (READ_ONCE(ewp->state) != STATE_NONE) {
>   			retval = 0;
>   			goto out_unlock;
>   		}
> @@ -1005,11 +1006,11 @@ static inline void __pipelined_op(struct
>   				  struct ext_wait_queue *this)
>   {
>   	list_del(&this->list);
> -	get_task_struct(this->task);
>   
> +	this->state = STATE_TRUE;
> +	wake_q_add(wake_q, this->task);
>   	/* see MQ_BARRIER for purpose/pairing */
>   	smp_store_release(&this->state, STATE_READY);
> -	wake_q_add_safe(wake_q, this->task);
>   }
>   
>   /* pipelined_send() - send a message directly to the task waiting in

As I have written in the other mail:

Copying this->task into a local variable is far simpler.

(before we had wake_q, an approach with 3 states was used, see e.g.: 
https://elixir.bootlin.com/linux/v2.6.39.4/source/ipc/msg.c#L858 : 
-EAGAIN, NULL, <valid pointer>. But due to the wake_q framework, we do 
not need that anymore)

-- 

     Manfred