David Howells wrote:
> Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> > > I'm also not entirely sure what 'paged' means in this function. Should it
> > > actually be set in the MSG_SPLICE_PAGES context?
> >
> > I introduced it with MSG_ZEROCOPY. It sets up pagedlen to capture the
> > length that is not copied.
> >
> > If the existing code would affect MSG_ZEROCOPY too, I expect syzbot
> > to have reported that previously.
>
> Ah... I think it *should* affect MSG_ZEROCOPY also... but... If you look at:
>
> } else {
> err = skb_zerocopy_iter_dgram(skb, from, copy);
> if (err < 0)
> goto error;
> }
> offset += copy;
> length -= copy;
>
> MSG_ZEROCOPY assumes that if it didn't return an error, then
> skb_zerocopy_iter_dgram() copied all the data requested - whether or not the
> iterator had sufficient data to copy.
>
> If you look in __zerocopy_sg_from_iter(), it will drop straight out, returning
> 0 if/when iov_iter_count() is/reaches 0, even if length is still > 0, just as
> skb_splice_from_iter() does.
>
> So there's a potential bug in the handling of MSG_ZEROCOPY - but one that you
> survive because it subtracts 'copy' from 'length', reducing it to zero, exits
> the loop and returns without looking at 'length' again. The actual length to
> be transmitted is in the skbuff.
>
> > Since the arithmetic is so complicated and error prone, I would try
> > to structure a fix that is easy to reason about to only change
> > behavior for the MSG_SPLICE_PAGES case.
>
> Does that mean you want to have a go at that - or did you want me to try
Please give it a try. I can review. It's just safer if it's trivial
to review that the patch only affects the behavior of the recently
introduced MSG_SPLICE_PAGES code.
