Messages in this thread | | | Date | Wed, 02 Aug 2023 10:25:40 -0400 | From | Willem de Bruijn <> | Subject | RE: [PATCH net-next] udp6: Fix __ip6_append_data()'s handling of MSG_SPLICE_PAGES |
| |
David Howells wrote: > __ip6_append_data() can has a similar problem to __ip_append_data()[1] when > asked to splice into a partially-built UDP message that has more than the > frag-limit data and up to the MTU limit, but in the ipv6 case, it errors > out with EINVAL. This can be triggered with something like: > > pipe(pfd); > sfd = socket(AF_INET6, SOCK_DGRAM, 0); > connect(sfd, ...); > send(sfd, buffer, 8137, MSG_CONFIRM|MSG_MORE); > write(pfd[1], buffer, 8); > splice(pfd[0], 0, sfd, 0, 0x4ffe0ul, 0); > > where the amount of data given to send() is dependent on the MTU size (in > this instance an interface with an MTU of 8192). > > The problem is that the calculation of the amount to copy in > __ip6_append_data() goes negative in two places, but a check has been put > in to give an error in this case. > > This happens because when pagedlen > 0 (which happens for MSG_ZEROCOPY and > MSG_SPLICE_PAGES), the terms in: > > copy = datalen - transhdrlen - fraggap - pagedlen; > > then mostly cancel when pagedlen is substituted for, leaving just -fraggap. > > Fix this by: > > (1) Insert a note about the dodgy calculation of 'copy'. > > (2) If MSG_SPLICE_PAGES, clear copy if it is negative from the above > equation, so that 'offset' isn't regressed and 'length' isn't > increased, which will mean that length and thus copy should match the > amount left in the iterator. > > (3) When handling MSG_SPLICE_PAGES, give a warning and return -EIO if > we're asked to splice more than is in the iterator. It might be > better to not give the warning or even just give a 'short' write. > > (4) If MSG_SPLICE_PAGES, override the copy<0 check. > > [!] Note that this should also affect MSG_ZEROCOPY, but that will return > -EINVAL for the range of send sizes that requires the skbuff to be split. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > cc: "David S. Miller" <davem@davemloft.net> > cc: Eric Dumazet <edumazet@google.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Paolo Abeni <pabeni@redhat.com> > cc: David Ahern <dsahern@kernel.org> > cc: Jens Axboe <axboe@kernel.dk> > cc: Matthew Wilcox <willy@infradead.org> > cc: netdev@vger.kernel.org > Link: https://lore.kernel.org/r/000000000000881d0606004541d1@google.com/ [1]
Reviewed-by: Willem de Bruijn <willemb@google.com>
I'm beginning to understand your point that the bug is older and copy should never end up equal to -fraglen. pagedlen includes all of datalen, which includes fraggap. This is wrong, as fraggap is always copied to skb->linear. Haven't really thought it through, but would this solve it as well?
else { alloclen = fragheaderlen + transhdrlen; - pagedlen = datalen - transhdrlen; + pagedlen = datalen - transhdrlen - fraggap;
After that copy no longer subtracts fraglen twice.
copy = datalen - transhdrlen - fraggap - pagedlen;
But don't mean to delay these targeted fixes for MSG_SPLICE_PAGES any further.
| |