linux-kernel - Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

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: <CAHk-=wjyHsGLx=rxg6PKYBNkPYAejgo7=CbyL3=HGLZLsAaJFQ@mail.gmail.com>
Date: Wed, 5 Mar 2025 06:40:38 -1000
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Mateusz Guzik <mjguzik@...il.com>, K Prateek Nayak <kprateek.nayak@....com>, 
	"Sapkal, Swapnil" <swapnil.sapkal@....com>, Manfred Spraul <manfred@...orfullife.com>, 
	Christian Brauner <brauner@...nel.org>, David Howells <dhowells@...hat.com>, 
	WangYuli <wangyuli@...ontech.com>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	"Shenoy, Gautham Ranjal" <gautham.shenoy@....com>, Neeraj.Upadhyay@....com, Ananth.narayan@....com
Subject: Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

On Mon, 3 Mar 2025 at 10:46, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> ENTIRELY UNTESTED, but it seems to generate ok code. It might even
> generate better code than what we have now.

Bah. This patch - which is now committed - was actually completely broken.

And the reason that complete breakage didn't show up in testing is
that I suspect nobody really tested or thought about the 32-bit case.

That whole "use 16-bit indexes on 32-bit" is all fine and well, but I
woke up in the middle of the night and realized that it doesn't
actually work.

Because now "pipe_occupancy()" is getting *entirely* the wrong
answers. It just does

        return head - tail;

but that only worked when the arithmetic was done modulo the size of
the indexes. And now it isn't.

So I still haven't *tested* this, but at an absolute minimum, we need
something like this:

  --- a/include/linux/pipe_fs_i.h
  +++ b/include/linux/pipe_fs_i.h
  @@ -192,7 +192,7 @@
    */
   static inline unsigned int pipe_occupancy(unsigned int head,
unsigned int tail)
   {
  -       return head - tail;
  +       return (pipe_index_t)(head - tail);
   }

   /**

and there might be other cases where the pipe_index_t size might matter.

For example, we should add a check to pipe_resize_ring() that the new
size is smaller than the index size. Yes, in practice 'pipe_max_size'
already ends up being that limit (the value is 256 pages), even for
16-bit indices, but we should do this properly.

And then, *while* looking at this, I also noticed that we had a very
much related bug in this area that was pre-existing and not related to
the 16-bit change: pipe_discard_from() is doing the wrong thing for
overflows even in the old 'unsigned int' type, and the whole

        while (pipe->head > old_head)

is bogus, because 'pipe->head' may have wrapped around, so the whole
"is it bigger" test doesn't work like that at all.

Of course, in practice it never hits (and would only hit more easily
with the new 16-bit thing), but it's very very wrong and can result in
a memory leak.

Are there other cases like this? I don't know. I've been looking
around a bit, but those were the only ones I found immediately when I
started thinking about the whole wrap-around issue.

I'd love it if other people tried to think about this too (and maybe
even test the 32-bit case - gasp!)

                         Linus