Re: [PATCH v3] ipc/mqueue: Avoid relying on a stack reference past its expiry
From: Davidlohr Bueso
Date: Sun May 09 2021 - 21:11:21 EST
On 2021-05-08 12:23, Manfred Spraul wrote:
On 5/7/21 3:38 PM, Varad Gautam wrote:
@@ -1005,11 +1022,9 @@ static inline void __pipelined_op(struct
struct ext_wait_queue *this)
+ wake_q_add(wake_q, this->task);
/* see MQ_BARRIER for purpose/pairing */
- wake_q_add_safe(wake_q, this->task);
/* pipelined_send() - send a message directly to the task waiting
First, I was too fast: I had assumed that wake_q_add() before
smp_store_release() would be a potential lost wakeup.
Yeah you need wake_up_q() to actually wake anything up.
As __pipelined_op() is called within spin_lock(&info->lock), and as
wq_sleep() will reread this->state after acquiring
spin_lock(&info->lock), I do not see a bug anymore.
Right, and when I proposed this version of the fix I was mostly focusing
being set as the last operation, but the fact of the matter is we had
moved to the
wake_q_add_safe() version for two reasons:
(1) Ensuring the ->state = STATE_READY is done after the reference count
racing with exit. In mqueue's original use of wake_q we were relying on
implied barrier from wake_q_add() in order to avoid reordering of
setting the state.
But this turned out to be insufficient hence the explicit
(2) In order to prevent a potential lost wakeup when the blocked task is
for wakeup by another task (the failed cmpxchg case in wake_q_add), and
therefore we need
to set the return condition (->state = STATE_READY) before adding the
task to the wake_q.
But I'm not seeing how race (2) can happen in mqueue. The race was
always theoretical to
begin with, with the exception of rwsems in which actually the wakee
task could end up in
the waker's wake_q without actually blocking.
So all in all I now agree that we should keep the order of how we
currently have things,
just to be on the safer side, if nothing else.