On Fri, Jan 13, 2017 at 10:20:19AM +0000, Al Viro wrote:
> OK, so it is iov_iter_advance() failing to free the shit allocated, either
> due to some breakage in pipe_advance() or buggered 'copied'... Let's
> see which one; could you apply the following and run your reproducer? The
> only difference from the previous is that it collects and prints a bit more,
> so it should be just as reproducible...
Actually, I think I understand what's going on. It's
if (pipe->nrbufs) {
int unused = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers -
1);
/* [curbuf,unused) is in use. Free [idx,unused) */
while (idx != unused) {
pipe_buf_release(pipe, &pipe->bufs[idx]);
idx = next_idx(idx, pipe);
pipe->nrbufs--;
}
}
in case when pipe->nrbufs == pipe->buffers and idx == pipe->curbuf. IOW,
we have a full pipe and want to empty it entirely; the fun question is,
of course, telling that case from having nothing to free with the same
full pipe...
OK, so we have either
* off != 0 => something's being left in the pipe, i->idx points
to the last in-use buffer after that, idx points to the first buffer unused
after that. In that case the current logics is correct.
* off == 0 => we are emptying the damn thing. i->idx and idx point
to the first buffer unused after that (== pipe->curbuf + pipe->nrbufs at the
time we'd set the iov_iter up). The current logics is correct unless
pipe->nrbufs was originally 0 and now has become pipe->buffers. IOW,
we screw up when off == 0, idx == unused, pipe->nrbufs == pipe->buffers...
OK, we really ought to make sure that iov_iter_pipe() is never
done on a full pipe. AFAICS, we do, and if so the following should
suffice. WARNING: it's completely untested and it's a result of debugging
a fencepost bug in handling of cyclic buffers done at 6 in the morning,
on _way_ too long uptime. Treat as very dangerous; it's not entirely
impossible that I hadn't fucked up, but don't consider it anything other
than "let's try and see if it immediately explodes" until I've got
a chance to reread it after getting some sleep.
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 25f572303801..7bc0b99d3c83 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -759,11 +759,12 @@ static void pipe_advance(struct iov_iter *i, size_t size)
idx = next_idx(idx, pipe);
if (pipe->nrbufs) {
int unused = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers -
1);
- /* [curbuf,unused) is in use. Free [idx,unused) */
- while (idx != unused) {
- pipe_buf_release(pipe, &pipe->bufs[idx]);
- idx = next_idx(idx, pipe);
- pipe->nrbufs--;
+ if (idx != unused || unlikely(idx == pipe->curbuf && !off)) {
+ do {
+ pipe_buf_release(pipe, &pipe->bufs[idx]);
+ idx = next_idx(idx, pipe);
+ pipe->nrbufs--;
+ } while (idx != unused);
}
}
i->count -= orig_sz;
@@ -826,6 +827,7 @@ void iov_iter_pipe(struct iov_iter *i, int direction,
size_t count)
{
BUG_ON(direction != ITER_PIPE);
+ WARN_ON(pipe->nrbufs == pipe->buffers);
i->type = direction;
i->pipe = pipe;
i->idx = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
