On Wed, Mar 05 2025, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
