[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230530173212.5fec9fc3@kernel.org>
Date: Tue, 30 May 2023 17:32:12 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
David Ahern <dsahern@...nel.org>,
Matthew Wilcox <willy@...radead.org>,
Jens Axboe <axboe@...nel.dk>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Chuck Lever <chuck.lever@...cle.com>,
Boris Pismenny <borisp@...dia.com>,
John Fastabend <john.fastabend@...il.com>,
Christoph Hellwig <hch@...radead.org>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: Bug in short splice to socket?
On Tue, 30 May 2023 23:26:43 +0100 David Howells wrote:
> Interesting. Now that you've pointed me at it, I've tried running it. Mostly
> it passes, but I'm having some problems with the multi_chunk_sendfile tests
> that time out. I think that splice_direct_to_actor() has a bug. The problem
> is this bit of code:
>
> /*
> * If more data is pending, set SPLICE_F_MORE
> * If this is the last data and SPLICE_F_MORE was not set
> * initially, clears it.
> */
> if (read_len < len)
> sd->flags |= SPLICE_F_MORE;
> else if (!more)
> sd->flags &= ~SPLICE_F_MORE;
>
> When used with sendfile(), it sets SPLICE_F_MORE (which causes MSG_MORE to be
> passed to the network protocol) if we haven't yet read everything that the
> user requested and clears it if we fulfilled what the user requested.
>
> This has the weird effect that MSG_MORE gets kind of inverted. It's never
> seen by the actor if we can read the entire request into the pipe - except if
> we hit the EOF first. If we hit the EOF before we fulfil the entire request,
> we get a short read and SPLICE_F_MORE and thus MSG_MORE *is* set. The
> upstream TLS code ignores it - but I'm changing this with my patches as
> sendmsg() then uses it to mark the EOR.
>
> I think we probably need to fix this in some way to check the size of source
> file - which may not be a regular file:-/ With the attached change, all tests
> pass; without it, a bunch of tests fail with timeouts.
Yeah.. it's one of those 'known warts' which we worked around in TLS
because I don't know enough about VFS to confidently fix it in fs/.
Proper fix would be pretty nice to have.
The original-original report of the problem is here, FWIW:
https://lore.kernel.org/netdev/1591392508-14592-1-git-send-email-pooja.trivedi@stackpath.com/
And my somewhat hacky fix was d452d48b9f8.
Powered by blists - more mailing lists