David Howells wrote: > __ip_append_data() can get into an infinite loop when asked to splice into > a partially-built UDP message that has more than the frag-limit data and up > to the MTU limit. Something like: > > pipe(pfd); > sfd = socket(AF_INET, SOCK_DGRAM, 0); > connect(sfd, ...); > send(sfd, buffer, 8161, 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 > __ip_append_data() goes negative in two places, and, in the second place, > this gets subtracted from the length remaining, thereby increasing it. > > This happens 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. > This causes: > > length -= copy + transhdrlen; > > to increase the length to more than the amount of data in msg->msg_iter, > which causes skb_splice_from_iter() to be unable to fill the request and it > returns less than 'copied' - which means that length never gets to 0 and we > never exit the loop. > > 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. > > The same problem occurs in __ip6_append_data(), except that there's a check > in there that errors out with EINVAL if copy < 0. Fix this function in > much the same way as the ipv4 variant but also skip the erroring out if > copy < 0. I don't think the two should be combined. Removing that branch actually opens up to new bugs, e.g., in MSG_ZEROCOPY. Since the ipv6 stack is not subject to this bug in MSG_SPLICE_PAGES, I think v1 on its own is correct, and has the right Fixes tag. If we think this IPv6 branch is mistaken and a bug, then that would have a different Fixes tag, going back to ancient history. But arguably safer to limit that net-next, if touching that at all. It makes sense to update MSG_SPLICE_PAGES in the IPv6 path to be equivalent to the IPv4 path, and to be correct if that branch is ever removed.
