Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
On Sun, Jun 12, 2022 at 5:10 PM Al Viro wrote: > > Unlike other copying operations on ITER_PIPE, copy_mc_to_iter() can > result in a short copy. In that case we need to trim the unused > buffers, as well as the length of partially filled one - it's not > enough to set ->head, ->iov_offset and ->count to reflect how > much had we copied. Not hard to fix, fortunately... > > I'd put a helper (pipe_discard_from(pipe, head)) into pipe_fs_i.h, > rather than iov_iter.c - Actually, since this "copy_mc_xyz()" stuff is going to be entirely impossible to debug and replicate for any normal situation, I would suggest we take the approach that we (long ago) used to take with copy_from_user(): zero out the destination buffer, so that developers that can't test the faulting behavior don't have to worry about it. And then the existing code is fine: it will break out of the loop, but it won't do the odd revert games and the "randomnoise.len -= rem" thing that I can't wrap my head around. Hmm? Linus
Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
On Mon, Jun 13, 2022 at 10:54:36AM -0700, Linus Torvalds wrote:
> On Sun, Jun 12, 2022 at 5:10 PM Al Viro wrote:
> >
> > Unlike other copying operations on ITER_PIPE, copy_mc_to_iter() can
> > result in a short copy. In that case we need to trim the unused
> > buffers, as well as the length of partially filled one - it's not
> > enough to set ->head, ->iov_offset and ->count to reflect how
> > much had we copied. Not hard to fix, fortunately...
> >
> > I'd put a helper (pipe_discard_from(pipe, head)) into pipe_fs_i.h,
> > rather than iov_iter.c -
>
> Actually, since this "copy_mc_xyz()" stuff is going to be entirely
> impossible to debug and replicate for any normal situation, I would
> suggest we take the approach that we (long ago) used to take with
> copy_from_user(): zero out the destination buffer, so that developers
> that can't test the faulting behavior don't have to worry about it.
>
> And then the existing code is fine: it will break out of the loop, but
> it won't do the odd revert games and the "randomnoise.len -= rem"
> thing that I can't wrap my head around.
>
> Hmm?
Not really - we would need to zero the rest of those pages somehow.
They are already allocated and linked into pipe; leaving them
there (and subsequent ones hadn't seen any stores whatsoever - they
are fresh out of alloc_page(GFP_USER)) is a non-starter.
We could do allocation as we go, but that's a much more intrusive
change...
BTW, speaking of pipes:
static inline unsigned int pipe_space_for_user(unsigned int head, unsigned int
tail,
struct pipe_inode_info *pipe)
{
unsigned int p_occupancy, p_space;
p_occupancy = pipe_occupancy(head, tail);
if (p_occupancy >= pipe->max_usage)
return 0;
p_space = pipe->ring_size - p_occupancy;
if (p_space > pipe->max_usage)
p_space = pipe->max_usage;
return p_space;
}
OK, if head - tail >= max_usage, we get 0. Fair enough, since
pipe_full() callers will get "it's full, sod off" in that situation.
But... what the hell is the rest doing? p_space is the amount of
slots not in use. So we return the lesser of it and max_usage?
Suppose we have 128 slots in the ring, with max_usage being below
that (e.g. 64). 63 slots are in use; you can add at most one.
And p_space is 65, so this sucker will return 64.
Dave, could you explain what's going on there? Note that pipe_write()
does *not* use that thing at all; it's only splice (i.e. ITER_PIPE
stuff) that is using it.
What's wrong with
p_occupancy = pipe_occupancy(head, tail);
if (p_occupancy >= pipe->max_usage)
return 0;
else
return pipe->max_usage - p_occupancy;
which would match the way you are using ->max_usage in pipe_write()
et.al. Including the use in copy_page_to_iter_pipe(), BTW...
Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
On Mon, Jun 13, 2022 at 11:28:34PM +0100, Al Viro wrote: > Dave, could you explain what's going on there? Note that pipe_write() > does *not* use that thing at all; it's only splice (i.e. ITER_PIPE > stuff) that is using it. > > What's wrong with > p_occupancy = pipe_occupancy(head, tail); > if (p_occupancy >= pipe->max_usage) > return 0; > else > return pipe->max_usage - p_occupancy; > > which would match the way you are using ->max_usage in pipe_write() > et.al. Including the use in copy_page_to_iter_pipe(), BTW... The more I'm looking at that thing, the more it smells like a bug; it had the same 3 callers since the time it had been introduced. 1) pipe_get_pages(). We are about to try and allocate up to that many pipe buffers. Allocation (done in push_pipe()) is done only if we have !pipe_full(pipe->head, pipe->tail, pipe->max_usage). It simply won't give you more than max_usage - occupancy. Your function returns min(ring_size - occupancy, max_usage), which is always greater than or equal to that (ring_size >= max_usage). 2) pipe_get_pages_alloc(). Same story, same push_pipe() being called, same "we'll never get that much - it'll hit the limit first". 3) iov_iter_npages() in case of ITER_PIPE. Again, the value is bogus - it should not be greater than the amount of pages we would be able to write there. AFAICS, 6718b6f855a0 "pipe: Allow pipes to have kernel-reserved slots" broke it for cases when ring_size != max_usage...
Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
On Tue, Jun 14, 2022 at 12:25:03AM +0100, Al Viro wrote:
> The more I'm looking at that thing, the more it smells like a bug;
> it had the same 3 callers since the time it had been introduced.
>
> 1) pipe_get_pages(). We are about to try and allocate up to that
> many pipe buffers. Allocation (done in push_pipe()) is done only
> if we have !pipe_full(pipe->head, pipe->tail, pipe->max_usage).
>
> It simply won't give you more than max_usage - occupancy.
> Your function returns min(ring_size - occupancy, max_usage), which
> is always greater than or equal to that (ring_size >= max_usage).
>
> 2) pipe_get_pages_alloc(). Same story, same push_pipe() being
> called, same "we'll never get that much - it'll hit the limit
> first".
>
> 3) iov_iter_npages() in case of ITER_PIPE. Again, the value
> is bogus - it should not be greater than the amount of pages
> we would be able to write there.
>
> AFAICS, 6718b6f855a0 "pipe: Allow pipes to have kernel-reserved slots"
> broke it for cases when ring_size != max_usage...
Unless I'm missing something, the following would do the right thing.
Dave?
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 4ea496924106..c22173d6e500 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -165,15 +165,10 @@ static inline bool pipe_full(unsigned int head, unsigned
int tail,
static inline unsigned int pipe_space_for_user(unsigned int head, unsigned int
tail,
struct pipe_inode_info *pipe)
{
- unsigned int p_occupancy, p_space;
-
- p_occupancy = pipe_occupancy(head, tail);
+ unsigned int p_occupancy = pipe_occupancy(head, tail);
if (p_occupancy >= pipe->max_usage)
return 0;
- p_space = pipe->ring_size - p_occupancy;
- if (p_space > pipe->max_usage)
- p_space = pipe->max_usage;
- return p_space;
+ return pipe->max_usage - p_occupancy;
}
/**
Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
On Mon, Jun 13, 2022 at 11:28:34PM +0100, Al Viro wrote: > On Mon, Jun 13, 2022 at 10:54:36AM -0700, Linus Torvalds wrote: > > On Sun, Jun 12, 2022 at 5:10 PM Al Viro wrote: > > > > > > Unlike other copying operations on ITER_PIPE, copy_mc_to_iter() can > > > result in a short copy. In that case we need to trim the unused > > > buffers, as well as the length of partially filled one - it's not > > > enough to set ->head, ->iov_offset and ->count to reflect how > > > much had we copied. Not hard to fix, fortunately... > > > > > > I'd put a helper (pipe_discard_from(pipe, head)) into pipe_fs_i.h, > > > rather than iov_iter.c - > > > > Actually, since this "copy_mc_xyz()" stuff is going to be entirely > > impossible to debug and replicate for any normal situation, I would > > suggest we take the approach that we (long ago) used to take with > > copy_from_user(): zero out the destination buffer, so that developers > > that can't test the faulting behavior don't have to worry about it. > > > > And then the existing code is fine: it will break out of the loop, but > > it won't do the odd revert games and the "randomnoise.len -= rem" > > thing that I can't wrap my head around. > > > > Hmm? > > Not really - we would need to zero the rest of those pages somehow. > They are already allocated and linked into pipe; leaving them > there (and subsequent ones hadn't seen any stores whatsoever - they > are fresh out of alloc_page(GFP_USER)) is a non-starter. > > We could do allocation as we go, but that's a much more intrusive > change... FWIW, I've got quite a bit of cleanups in the local tree; reordering and cleaning that queue up at the moment, will post tonight or tomorrow. I've looked into doing allocations page-by-page (instead of single push_pipe(), followed by copying into those). Doable, but it ends up being much messier. IMO this "truncate on failure" approach is saner.
Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()
Al Viro wrote: > What's wrong with > p_occupancy = pipe_occupancy(head, tail); > if (p_occupancy >= pipe->max_usage) > return 0; > else > return pipe->max_usage - p_occupancy; Because "pipe->max_usage - p_occupancy" can be negative. post_one_notification() is limited by pipe->ring_size, not pipe->max_usage. The idea is to allow some slack in a watch pipe for the watch_queue code to use that userspace can't. David
