linux-kernel - Re: wakeup_pipe_readers/writers() && pipe

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: <42f82db0-f252-4973-9f27-664286c84392@colorfullife.com>
Date: Sat, 4 Jan 2025 21:57:49 +0100
From: Manfred Spraul <manfred@...orfullife.com>
To: Oleg Nesterov <oleg@...hat.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Christian Brauner <brauner@...nel.org>, David Howells <dhowells@...hat.com>
Cc: WangYuli <wangyuli@...ontech.com>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: wakeup_pipe_readers/writers() && pipe_poll()

Hi Oleg,

On 1/2/25 5:33 PM, Oleg Nesterov wrote:
> I was going to send a one-liner patch which adds mb() into pipe_poll()
> but then I decided to make even more spam and ask some questions first.
>
> 	static void wakeup_pipe_readers(struct pipe_inode_info *pipe)
> 	{
> 		smp_mb();
> 		if (waitqueue_active(&pipe->rd_wait))
> 			wake_up_interruptible(&pipe->rd_wait);
> 		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> 	}
>
> I think that wq_has_sleeper() + wake_up_interruptible_poll(POLLIN) make more
> sense but this is minor.
>
> Either way the waitqueue_active() check is only correct if the waiter has a
> barrier between __add_wait_queue() and "check the condition". wait_event()
> is fine, but pipe_poll() does:
>
> 	// poll_wait()
> 	__pollwait() -> add_wait_queue(pipe->rd_wait) -> list_add()
>
> 	READ_ONCE(pipe->head);
> 	READ_ONCE(pipe->tail);
>
> In theory these LOAD's can leak into the critical section in add_wait_queue()
> and they can happen before list_add(entry, rd_wait.head).
>
> So I think we need the trivial
>
> 	--- a/fs/pipe.c
> 	+++ b/fs/pipe.c
> 	@@ -680,6 +680,7 @@ pipe_poll(struct file *filp, poll_table *wait)
> 		 * if something changes and you got it wrong, the poll
> 		 * table entry will wake you up and fix it.
> 		 */
> 	+	smp_mb();
> 		head = READ_ONCE(pipe->head);
> 		tail = READ_ONCE(pipe->tail);
>
> and after that pipe_read/pipe_write can use the wq_has_sleeper() check too
> (this is what the patch from WangYuli did).

Would it be possible to create a perf probe to get some statistics?

I see at least 4 options:

- do nothing

- add the smp_mb() into pipe_poll, and convert pipe to wq_has_sleepers()

- add the smp_mb() into poll_wait(), convert pipe and potentially 
further poll users to wq_has_sleepers()

- add the smp_mb() into __add_wait_queue(), and merge wq_has_sleepers() 
into wake_up().

The tricky part is probably to differentiate wake_up on empty wait 
queues vs. wake_up on wait queues with entries.

--

     Manfred