From: Paolo Abeni <pabeni@redhat.com>
To: Sascha Hauer <s.hauer@pengutronix.de>, netdev@vger.kernel.org
Cc: kernel@pengutronix.de, linux-kernel@vger.kernel.org,
Jens Axboe <axboe@kernel.dk>,
io-uring@vger.kernel.org
Subject: Re: [PATCH] net: Do not break out of sk_stream_wait_memory() with TIF_NOTIFY_SIGNAL
Date: Tue, 19 Mar 2024 13:30:26 +0100 [thread overview]
Message-ID: <9c63cf0c31792270026fc673334aa76f855eae35.camel@redhat.com> (raw)
In-Reply-To: <ZfgtgwEM69VPJGs7@pengutronix.de>
On Mon, 2024-03-18 at 13:03 +0100, Sascha Hauer wrote:
> Apologies, I have sent the wrong mail. Here is the mail I really wanted
> to send, with answers to some of the questions Paolo raised the last
> time I sent it.
>
> -----------------------------------8<------------------------------
>
> > From 566bb198546423c024cdebc50d0aade7ed638a40 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 23 Oct 2023 14:13:46 +0200
> Subject: [PATCH v2] net: Do not break out of sk_stream_wait_memory() with TIF_NOTIFY_SIGNAL
>
> It can happen that a socket sends the remaining data at close() time.
> With io_uring and KTLS it can happen that sk_stream_wait_memory() bails
> out with -512 (-ERESTARTSYS) because TIF_NOTIFY_SIGNAL is set for the
> current task. This flag has been set in io_req_normal_work_add() by
> calling task_work_add().
>
> It seems signal_pending() is too broad, so this patch replaces it with
> task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
>
> A discussion of this issue can be found at
> https://lore.kernel.org/20231010141932.GD3114228@pengutronix.de
>
> Suggested-by: Jens Axboe <axboe@kernel.dk>
> Fixes: 12db8b690010c ("entry: Add support for TIF_NOTIFY_SIGNAL")
> Link: https://lore.kernel.org/r/20231023121346.4098160-1-s.hauer@pengutronix.de
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>
> Changes since v1:
> - only replace signal_pending() with task_sigpending() where we need it,
> in sk_stream_wait_memory()
>
> I'd like to pick up the discussion on this patch as it is still needed for our
> usecase. Paolo Abeni raised some concerns about this patch for which I didn't have
> good answers. I am referencing them here again with an attempts to answer them.
> Jens, maybe you also have a few words here.
>
> Paolo raised some concerns in
> https://lore.kernel.org/all/e1e15554bfa5cfc8048d6074eedbc83c4d912c98.camel@redhat.com/:
>
> > To be more explicit: why this will not cause user-space driven
> > connect() from missing relevant events?
>
> Note I dropped the hunk in sk_stream_wait_connect() and
> sk_stream_wait_close() in this version.
> Userspace driven signals are still catched with task_sigpending() which
> tests for TIF_SIGPENDING. signal_pending() will additionally check for
> TIF_NOTIFY_SIGNAL which is exclusively used by task_work_add() to add
> work to a task.
It looks like even e.g. livepatch would set TIF_NOTIFY_SIGNAL, and
ignoring it could break livepatch for any code waiting e.g. in
tcp_sendmsg()?!?
This change looks scary to me.
I think what Pavel is suggesting is to refactor the KTLS code to ensure
all the writes are completed before releasing the last socket
reference.
I would second such suggestion.
If really nothing else works, and this change is the only option, try
to obtain an ack from kernel/signal.c maintainers.
Thanks,
Paolo
next prev parent reply other threads:[~2024-03-19 12:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-15 10:01 [PATCH] net: Do not break out of sk_stream_wait_memory() with TIF_NOTIFY_SIGNAL Sascha Hauer
2024-03-15 16:43 ` Jens Axboe
2024-03-15 17:02 ` Pavel Begunkov
2024-03-18 12:10 ` Sascha Hauer
2024-03-18 13:19 ` Pavel Begunkov
2024-03-19 10:50 ` Sascha Hauer
2024-03-19 13:55 ` Pavel Begunkov
2024-03-19 15:08 ` Sascha Hauer
2024-03-18 12:03 ` Sascha Hauer
2024-03-19 12:30 ` Paolo Abeni [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-10-23 12:13 Sascha Hauer
2023-10-24 13:56 ` Paolo Abeni
2023-10-26 7:03 ` Sascha Hauer
2023-10-26 8:49 ` Paolo Abeni
2023-10-27 12:04 ` Sascha Hauer
2023-11-17 10:43 ` Marc Kleine-Budde
2023-12-19 11:00 ` Steffen Trumtrar
2023-12-19 13:13 ` Paolo Abeni
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=9c63cf0c31792270026fc673334aa76f855eae35.camel@redhat.com \
--to=pabeni@redhat.com \
--cc=axboe@kernel.dk \
--cc=io-uring@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).