[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <64c901683e0b6_1b28392946b@willemb.c.googlers.com.notmuch>
Date: Tue, 01 Aug 2023 08:58:16 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: David Howells <dhowells@...hat.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: dhowells@...hat.com, Jakub Kicinski <kuba@...nel.org>,
syzbot <syzbot+f527b971b4bdc8e79f9e@...kaller.appspotmail.com>,
bpf@...r.kernel.org, brauner@...nel.org, davem@...emloft.net,
dsahern@...nel.org, edumazet@...gle.com,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, pabeni@...hat.com,
syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk
Subject: Re: Endless loop in udp with MSG_SPLICE_READ - Re: [syzbot] [fs?]
INFO: task hung in pipe_release (4)
David Howells wrote:
> The more I look at __ip_append_data(), the more I think the maths is wrong.
> In the bit that allocates a new skbuff:
>
> if (copy <= 0) {
> ...
> datalen = length + fraggap;
> if (datalen > mtu - fragheaderlen)
> datalen = maxfraglen - fragheaderlen;
> fraglen = datalen + fragheaderlen;
> pagedlen = 0;
> ...
> if ((flags & MSG_MORE) &&
> !(rt->dst.dev->features&NETIF_F_SG))
> ...
> else if (!paged &&
> (fraglen + alloc_extra < SKB_MAX_ALLOC ||
> !(rt->dst.dev->features & NETIF_F_SG)))
> ...
> else {
> alloclen = fragheaderlen + transhdrlen;
> pagedlen = datalen - transhdrlen;
> }
> ...
>
> In the MSG_SPLICE_READ but not MSG_MORE case, we go through that else clause.
> The values used here, a few lines further along:
>
> copy = datalen - transhdrlen - fraggap - pagedlen;
>
> are constant over the intervening span. This means that, provided the splice
> isn't going to exceed the MTU on the second fragment, the calculation of
> 'copy' can then be simplified algebraically thus:
>
> copy = (length + fraggap) - transhdrlen - fraggap - pagedlen;
>
> copy = length - transhdrlen - pagedlen;
>
> copy = length - transhdrlen - (datalen - transhdrlen);
>
> copy = length - transhdrlen - datalen + transhdrlen;
>
> copy = length - datalen;
>
> copy = length - (length + fraggap);
>
> copy = length - length - fraggap;
>
> copy = -fraggap;
>
> I think we might need to recalculate copy after the conditional call to
> getfrag(). Possibly we should skip that entirely for MSG_SPLICE_READ. The
> root seems to be that we're subtracting pagedlen from datalen - but probably
> we shouldn't be doing getfrag() if pagedlen > 0.
q
Powered by blists - more mailing lists