Re: [PATCH] ipc/mqueue: Avoid relying on a stack reference past its expiry

[Varad Gautam]

Hey Manfred,

On 5/8/21 7:12 PM, Manfred Spraul wrote:
> Hi Varad,
> 
> On 5/4/21 5:55 PM, Varad Gautam wrote:
>> do_mq_timedsend::__pipelined_op() should not dereference `this` after
>> setting STATE_READY, as the receiver counterpart is now free to return.
>> Change __pipelined_op to call wake_q_add_safe on the receiver's
>> task_struct returned by get_task_struct, instead of dereferencing
>> `this` which sits on the receiver's stack.
> Correct. I was so concentrated on the risks of reordered memory that I have overlooked the simple bug.
>> Fixes: c5b2cbdbdac563 ("ipc/mqueue.c: update/document memory barriers")
> Actually, sem.c and msg.c contain the same bug. Thus all three must be fixed.

You're right, it's the same usage pattern.

>> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
>> Reported-by: Matthias von Faber <matthias.vonfaber@xxxxxxxxxxx>
>> Cc: Christian Brauner <christian.brauner@xxxxxxxxxx>
>> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
>> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>> Cc: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Davidlohr Bueso <dbueso@xxxxxxx>
>>
>> ---
>>   ipc/mqueue.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 8031464ed4ae2..8f78057c6be53 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -1004,12 +1004,14 @@ static inline void __pipelined_op(struct wake_q_head *wake_q,
>>                     struct mqueue_inode_info *info,
>>                     struct ext_wait_queue *this)
>>   {
>> +    struct task_struct *t;
>> +
>>       list_del(&this->list);
>> -    get_task_struct(this->task);
>> +    t = get_task_struct(this->task);
>>         /* see MQ_BARRIER for purpose/pairing */
>>       smp_store_release(&this->state, STATE_READY);
>> -    wake_q_add_safe(wake_q, this->task);
>> +    wake_q_add_safe(wake_q, t);
>>   }
> 
> The change fixes the issue, but I would prefer to use t = this->task instead of using the return value of get_task_struct():
> Then all wake_q_add_safe() users are identical.
> 
> Ok for you?
> 
> Slightly tested patch attached.

Thanks, I've sent out a v4 at [1] integrating sem.c/msg.c. Note that I went with
context-local naming and used what get_task_struct returns as I don't see much
difference either way.

[1] https://lore.kernel.org/lkml/20210510102950.12551-1-varad.gautam@xxxxxxxx/

Thanks,
Varad

> 
> -- 
> 
>     Manfred
> 

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany

HRB 36809, AG Nürnberg
Geschäftsführer: Felix Imendörffer