[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <64c91dd7ebb09_1c09b429484@willemb.c.googlers.com.notmuch>
Date: Tue, 01 Aug 2023 10:59:35 -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:
> Willem de Bruijn <willemdebruijn.kernel@...il.com> 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.
Powered by blists - more mailing lists