Re: [PATCH] net: Do not break out of sk_stream_wait_memory() with TIF_NOTIFY_SIGNAL

From: Pavel Begunkov
Date: Mon Mar 18 2024 - 09:21:09 EST


On 3/18/24 12:10, Sascha Hauer wrote:
On Fri, Mar 15, 2024 at 05:02:05PM +0000, Pavel Begunkov wrote:
On 3/15/24 10:01, Sascha Hauer wrote:
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().

The entire idea of task_work is to interrupt syscalls and let io_uring
do its job, otherwise it wouldn't free resources it might be holding,
and even potentially forever block the syscall.

I'm not that sure about connect / close (are they not restartable?),
but it doesn't seem to be a good idea for sk_stream_wait_memory(),
which is the normal TCP blocking send path. I'm thinking of some kinds
of cases with a local TCP socket pair, the tx queue is full as well
and the rx queue of the other end, and io_uring has to run to receive
the data.

There is another case, let's say the IO is done via io-wq
(io_uring's worker thread pool) and hits the waiting. Now the
request can't get cancelled, which is done by interrupting the
task with TIF_NOTIFY_SIGNAL. User requested request cancellations
is one thing, but we'd need to check if io_uring can ever be closed
in this case.


If interruptions are not welcome you can use different io_uring flags,
see IORING_SETUP_COOP_TASKRUN and/or IORING_SETUP_DEFER_TASKRUN.

I tried with different combinations of these flags. For example
IORING_SETUP_TASKRUN_FLAG | IORING_SETUP_SINGLE_ISSUER | IORING_SETUP_DEFER_TASKRUN
makes the issue less likely, but nevertheless it still happens.

However, reading the documentation of these flags, they shall provide
hints to the kernel for optimizations, but it should work without these
flags, right?

That's true, and I guess there are other cases as well, like
io-wq and perhaps even a stray fput.


Maybe I'm missing something, why not restart your syscall?

The problem comes with TLS. Normally with synchronous encryption all
data on a socket is written during write(). When asynchronous
encryption comes into play, then not all data is written during write(),
but instead the remaining data is written at close() time.

Was it considered to do the final cleanup in workqueue
and only then finalising the release?


Here is my call stack when things go awry:

[ 325.560946] tls_push_sg: tcp_sendmsg_locked returned -512
[ 325.566371] CPU: 1 PID: 305 Comm: webserver_libur Not tainted 6.8.0-rc6-00022-g932acd9c444b-dirty #248
[ 325.575684] Hardware name: NXP i.MX8MPlus EVK board (DT)
[ 325.580997] Call trace:
[ 325.583444] dump_backtrace+0x90/0xe8
[ 325.587122] show_stack+0x18/0x24
[ 325.590444] dump_stack_lvl+0x48/0x60
[ 325.594114] dump_stack+0x18/0x24
[ 325.597432] tls_push_sg+0xfc/0x22c
[ 325.600930] tls_tx_records+0x114/0x1cc
[ 325.604772] tls_sw_release_resources_tx+0x3c/0x140
[ 325.609658] tls_sk_proto_close+0x2b0/0x3ac
[ 325.613846] inet_release+0x4c/0x9c
[ 325.617341] __sock_release+0x40/0xb4
[ 325.621007] sock_close+0x18/0x28
[ 325.624328] __fput+0x70/0x2bc
[ 325.627386] ____fput+0x10/0x1c
[ 325.630531] task_work_run+0x74/0xcc
[ 325.634113] do_notify_resume+0x22c/0x1310
[ 325.638220] el0_svc+0xa4/0xb4
[ 325.641279] el0t_64_sync_handler+0x120/0x12c
[ 325.645643] el0t_64_sync+0x190/0x194

As said, TLS is sending remaining data at close() time in tls_push_sg().
Here sk_stream_wait_memory() gets interrupted and returns -ERESTARTSYS.
There's no way to restart this operation, the socket is about to be
closed and won't accept data anymore.

--
Pavel Begunkov