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

[Rasmus Villemoes <ravi@prevas.dk>]

On Wed, Mar 05 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 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.

Without too much brainpower spent analyzing each case (i.e., some of
these might actually be ok), I found these:

fs/fuse/dev.c
fuse_dev_splice_write()

  unsigned int head, tail, mask, count;

  pipe_lock(pipe);

  head = pipe->head;
  tail = pipe->tail;
  mask = pipe->ring_size - 1;
  count = head - tail;

Open-coded pipe_occupancy(), so would be fixed by using that with your
fixup.

A bit later in same function there's the same FIONREAD pattern:

  for (idx = tail; idx != head && rem < len; idx++)
          rem += pipe->bufs[idx & mask].len;


fs/pipe.c

We have pipe_update_tail() getting and returning an "unsigned int",
and letting the compiler truncate the result written to pipe->tail:

      pipe->tail = ++tail;
      return tail;

pipe_update_tail() only has one caller, but a rather important one,
pipe_read(), which uses the return value from pipe_update_tail as-is

                 tail = pipe_update_tail(pipe, buf, tail);
         }
         total_len -= chars;
         if (!total_len)
                 break;  /* common path: read succeeded */
         if (!pipe_empty(head, tail))    /* More to do? */
                 continue;

and pipe_empty() takes two "unsigned ints" and is just head==tail -- so if
tail was incremented to 65536 while head is 0 that would break. Probably
pipe_empty() should either take pipe_index_t arguments or cast to that
internally, just as pipe_occupancy. Or, as pipe_full(), being spelled in
terms of pipe_occupancy()==0.

With that fixed, maybe one could spell the FIONREAD-like patterns using
pipe_empty(), i.e. using pipe_empty() to ask "have this tail index now
caught up to this head index". So "idx != head" above would become
"!pipe_empty(idx, head)".

Rasmus