linux-kernel - Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

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]

Message-ID: <a71a7cad-c007-45be-9fd1-22642b835edd@colorfullife.com>
Date: Sun, 29 Dec 2024 20:54:40 +0100
From: Manfred Spraul <manfred@...orfullife.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 WangYuli <wangyuli@...ontech.com>,
 linux-fsdevel <linux-fsdevel@...r.kernel.org>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 Christian Brauner <brauner@...nel.org>
Subject: Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping
 processes during pipe read/write

Hi Oleg,

On 12/29/24 2:13 PM, Oleg Nesterov wrote:
> Sorry for the noise...
>
> and currently this is fine. But if we want to add the wq_has_sleeper()
> checks into fs/pipe.c then pipe_poll() needs smp_mb() after it calls
> poll_wait().
>
> Agreed?

Yes, agreed.

Just the comment in pipe_poll() was a bit tricky for me.

https://elixir.bootlin.com/linux/v6.12.6/source/fs/pipe.c#L670

If i understand it right, from memory ordering perspective, it is 
undefined if pipe->head/tail is written before or after updating the 
linked list in the poll table entry.

But: Reading cannot happen before taking the spinlock.

CPU1:

pipe_poll()-> poll_wait()->__pollwait()->add_wait_queue()->spin_lock()
LOAD(pipe->head, pipe->tail)
<<<<< insert here CPU2
<add to wait queue>
pipe_poll()-> poll_wait()->__pollwait()->add_wait_queue()->spin_unlock()
<use pipe->head, pipe->tail>

CPU2:

pipe_update_tail()
mutex_unlock()
wake_up_interruptible_sync_poll
spin_lock() >>>>> this closes the potential race

perhaps:

     /*
      * .. and only then can you do the racy tests. That way,
      * if something changes and you got it wrong, the poll
      * table entry will wake you up and fix it.

+   * The memory barrier for this READ_ONCE is provided by

+   * poll_wait()->__pollwait()->add_wait_queue()->spin_lock()

+ * It pairs with the spin_lock() in wake_up

      */
     head = READ_ONCE(pipe->head);

> On 12/29, Oleg Nesterov wrote:
>> On 12/29, Manfred Spraul wrote:
>>>> I think that your patch (and the original patch from WangYuli) has the same
>>>> proble with pipe_poll()->poll_wait()->__pollwait().
>>> What is the memory barrier for pipe_poll()?
>>>
>>> There is poll_wait()->__pollwait()->add_wait_queue()->spin_unlock(). thus
>>> only store_release.
>>>
>>> And then READ_ONCE(), i.e. no memory barrier.
>>>
>>> Thus the CPU would be free to load pipe->head and pipe->tail before adding
>>> the entry to the poll table.
>>>
>>> Correct?
>> Yes, this was my thinking.
>>
>> See also my initial reply to WangYuli
>> https://lore.kernel.org/all/20241226160007.GA11118@redhat.com/
>>
>> Do you agree?
>>
>> Oleg.