All of lore.kernel.org
 help / color / mirror / Atom feed
From: Seth Forshee <seth.forshee@canonical.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: davem@davemloft.net, netdev@vger.kernel.org, borisp@nvidia.com,
	john.fastabend@gmail.com, daniel@iogearbox.net,
	davejwatson@fb.com, ilyal@mellanox.com, aviadye@mellanox.com,
	Vadim Fedorenko <vfedorenko@novek.ru>
Subject: Re: [PATCH net] tls: prevent oversized sendfile() hangs by ignoring MSG_MORE
Date: Fri, 18 Jun 2021 16:04:07 -0500	[thread overview]
Message-ID: <YM0KR8IPGoSBgCl8@ubuntu-x1> (raw)
In-Reply-To: <20210618203406.1437414-1-kuba@kernel.org>

On Fri, Jun 18, 2021 at 01:34:06PM -0700, Jakub Kicinski wrote:
> We got multiple reports that multi_chunk_sendfile test
> case from tls selftest fails. This was sort of expected,
> as the original fix was never applied (see it in the first
> Link:). The test in question uses sendfile() with count
> larger than the size of the underlying file. This will
> make splice set MSG_MORE on all sendpage calls, meaning
> TLS will never close and flush the last partial record.
> 
> Eric seem to have addressed a similar problem in
> commit 35f9c09fe9c7 ("tcp: tcp_sendpages() should call tcp_push() once")
> by introducing MSG_SENDPAGE_NOTLAST. Unlike MSG_MORE
> MSG_SENDPAGE_NOTLAST is not set on the last call
> of a "pipefull" of data (PIPE_DEF_BUFFERS == 16,
> so every 16 pages or whenever we run out of data).
> 
> Having a break every 16 pages should be fine, TLS
> can pack exactly 4 pages into a record, so for
> aligned reads there should be no difference,
> unaligned may see one extra record per sendpage().
> 
> Sticking to TCP semantics seems preferable to modifying
> splice, but we can revisit it if real life scenarios
> show a regression.
> 
> Reported-by: Vadim Fedorenko <vfedorenko@novek.ru>
> Reported-by: Seth Forshee <seth.forshee@canonical.com>
> Link: https://lore.kernel.org/netdev/1591392508-14592-1-git-send-email-pooja.trivedi@stackpath.com/
> Fixes: 3c4d7559159b ("tls: kernel TLS support")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

With this patch the muilt_chunk_sendfile selftest passes. Thanks!

Tested-by: Seth Forshee <seth.forshee@canonical.com>

  reply	other threads:[~2021-06-18 21:04 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18 20:34 [PATCH net] tls: prevent oversized sendfile() hangs by ignoring MSG_MORE Jakub Kicinski
2021-06-18 21:04 ` Seth Forshee [this message]
2021-06-21 19:50 ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YM0KR8IPGoSBgCl8@ubuntu-x1 \
    --to=seth.forshee@canonical.com \
    --cc=aviadye@mellanox.com \
    --cc=borisp@nvidia.com \
    --cc=daniel@iogearbox.net \
    --cc=davejwatson@fb.com \
    --cc=davem@davemloft.net \
    --cc=ilyal@mellanox.com \
    --cc=john.fastabend@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vfedorenko@novek.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.