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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241229115439.GA27491@redhat.com>
Date: Sun, 29 Dec 2024 12:54:40 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Manfred Spraul <manfred@...orfullife.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 Manfred,

Sorry, I don't understand, perhaps you misunderstood me too.

On 12/28, Manfred Spraul wrote:
>
> >Even simpler,
> >
> >	void wait(void)
> >	{
> >		DEFINE_WAIT(entry);
> >
> >		__set_current_state(XXX);
> >		add_wait_queue(WQ, entry);
> >
> >		if (!CONDITION)
> >			schedule();
> >
> >		remove_wait_queue(WQ, entry);
> >		__set_current_state(TASK_RUNNING);
> >	}
> >
> >This code is ugly but currently correct unless I am totally confused.

What I tried to say: the code above is another (simpler) example of
the currently correct (afaics) code which will be broken by your patch.

Of course, wait() assumes that

	void wake(void)
	{
		CONDITION = 1;
		wake_up(WQ);
	}

calls __wake_up_common_lock() and takes WQ->lock unconditionally, and
thus wait() doesn't need the additional barries.

> And: Your proposal is in conflict with
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/fork.c?h=v2.6.0&id=e220fdf7a39b54a758f4102bdd9d0d5706aa32a7

I proposed nothing ;) But yes sure, this code doesn't match the comment
above waitqueue_active(), and that is why the wake() paths can't check
list_empty() to avoid __wake_up_common_lock().

> But I do not see the issue, the worst possible scenario should be something like:
>
> 	// add_wait_queue
> 		spin_lock(WQ->lock);
> 		LOAD(CONDITION);	// false!
> 		list_add(entry, head);
> 		STORE(current_state)
> 		spin_unlock(WQ->lock);

Again, wake() can happen between LOAD() and list_add()...

But sorry again, I guess I completely misunderstood you...

Oleg.