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?
David
