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/[email protected]
Suggested-by: Jens Axboe <[email protected]>
Fixes: 12db8b690010c ("entry: Add support for TIF_NOTIFY_SIGNAL")
Signed-off-by: Sascha Hauer <[email protected]>
---
net/core/stream.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/core/stream.c b/net/core/stream.c
index 96fbcb9bbb30a..e9e17b48e0122 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -67,7 +67,7 @@ int sk_stream_wait_connect(struct sock *sk, long *timeo_p)
return -EPIPE;
if (!*timeo_p)
return -EAGAIN;
- if (signal_pending(tsk))
+ if (task_sigpending(tsk))
return sock_intr_errno(*timeo_p);
add_wait_queue(sk_sleep(sk), &wait);
@@ -103,7 +103,7 @@ void sk_stream_wait_close(struct sock *sk, long timeout)
do {
if (sk_wait_event(sk, &timeout, !sk_stream_closing(sk), &wait))
break;
- } while (!signal_pending(current) && timeout);
+ } while (!task_sigpending(current) && timeout);
remove_wait_queue(sk_sleep(sk), &wait);
}
@@ -134,7 +134,7 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
goto do_error;
if (!*timeo_p)
goto do_eagain;
- if (signal_pending(current))
+ if (task_sigpending(current))
goto do_interrupted;
sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
if (sk_stream_memory_free(sk) && !vm_wait)
--
2.39.2
On Mon, 2023-10-23 at 14:13 +0200, 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().
>
> It seems signal_pending() is too broad, so this patch replaces it with
> task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
This looks dangerous, at best. Other possible legit users setting
TIF_NOTIFY_SIGNAL will be broken.
Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
Thanks!
Paolo
On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
> On Mon, 2023-10-23 at 14:13 +0200, 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().
> >
> > It seems signal_pending() is too broad, so this patch replaces it with
> > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
>
> This looks dangerous, at best. Other possible legit users setting
> TIF_NOTIFY_SIGNAL will be broken.
>
> Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
I don't have an idea how io_run_task_work() comes into play here, but it
seems it already clears TIF_NOTIFY_SIGNAL:
static inline int io_run_task_work(void)
{
/*
* Always check-and-clear the task_work notification signal. With how
* signaling works for task_work, we can find it set with nothing to
* run. We need to clear it for that case, like get_signal() does.
*/
if (test_thread_flag(TIF_NOTIFY_SIGNAL))
clear_notify_signal();
...
}
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, 2023-10-26 at 09:03 +0200, Sascha Hauer wrote:
> On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
> > On Mon, 2023-10-23 at 14:13 +0200, 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().
> > >
> > > It seems signal_pending() is too broad, so this patch replaces it with
> > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
> >
> > This looks dangerous, at best. Other possible legit users setting
> > TIF_NOTIFY_SIGNAL will be broken.
> >
> > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
>
> I don't have an idea how io_run_task_work() comes into play here, but it
> seems it already clears TIF_NOTIFY_SIGNAL:
>
> static inline int io_run_task_work(void)
> {
> /*
> * Always check-and-clear the task_work notification signal. With how
> * signaling works for task_work, we can find it set with nothing to
> * run. We need to clear it for that case, like get_signal() does.
> */
> if (test_thread_flag(TIF_NOTIFY_SIGNAL))
> clear_notify_signal();
> ...
> }
I see, io_run_task_work() is too late, sk_stream_wait_memory() is
already woken up.
I still think this patch is unsafe. What about explicitly handling the
restart in tls_sw_release_resources_tx() ? The main point is that such
function is called by inet_release() and the latter can't be re-
started.
Cheers,
Paolo
On Thu, Oct 26, 2023 at 10:49:18AM +0200, Paolo Abeni wrote:
> On Thu, 2023-10-26 at 09:03 +0200, Sascha Hauer wrote:
> > On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
> > > On Mon, 2023-10-23 at 14:13 +0200, 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().
> > > >
> > > > It seems signal_pending() is too broad, so this patch replaces it with
> > > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
> > >
> > > This looks dangerous, at best. Other possible legit users setting
> > > TIF_NOTIFY_SIGNAL will be broken.
> > >
> > > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
> >
> > I don't have an idea how io_run_task_work() comes into play here, but it
> > seems it already clears TIF_NOTIFY_SIGNAL:
> >
> > static inline int io_run_task_work(void)
> > {
> > /*
> > * Always check-and-clear the task_work notification signal. With how
> > * signaling works for task_work, we can find it set with nothing to
> > * run. We need to clear it for that case, like get_signal() does.
> > */
> > if (test_thread_flag(TIF_NOTIFY_SIGNAL))
> > clear_notify_signal();
> > ...
> > }
>
> I see, io_run_task_work() is too late, sk_stream_wait_memory() is
> already woken up.
>
> I still think this patch is unsafe. What about explicitly handling the
> restart in tls_sw_release_resources_tx() ? The main point is that such
> function is called by inet_release() and the latter can't be re-
> started.
I don't think there's anything I can do in tls_sw_release_resources_tx().
When entering this function TIF_NOTIFY_SIGNAL is not (yet) set. It gets
set at some point while tls_sw_release_resources_tx() is running. I find
it set when tls_tx_records() returns with -ERESTARTSYS. I tried clearing
TIF_NOTIFY_SIGNAL then and called tls_tx_records() again, but that doesn't
work.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 27.10.2023 14:04:32, Sascha Hauer wrote:
> On Thu, Oct 26, 2023 at 10:49:18AM +0200, Paolo Abeni wrote:
> > On Thu, 2023-10-26 at 09:03 +0200, Sascha Hauer wrote:
> > > On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
> > > > On Mon, 2023-10-23 at 14:13 +0200, 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().
> > > > >
> > > > > It seems signal_pending() is too broad, so this patch replaces it with
> > > > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
> > > >
> > > > This looks dangerous, at best. Other possible legit users setting
> > > > TIF_NOTIFY_SIGNAL will be broken.
> > > >
> > > > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
> > >
> > > I don't have an idea how io_run_task_work() comes into play here, but it
> > > seems it already clears TIF_NOTIFY_SIGNAL:
> > >
> > > static inline int io_run_task_work(void)
> > > {
> > > /*
> > > * Always check-and-clear the task_work notification signal. With how
> > > * signaling works for task_work, we can find it set with nothing to
> > > * run. We need to clear it for that case, like get_signal() does.
> > > */
> > > if (test_thread_flag(TIF_NOTIFY_SIGNAL))
> > > clear_notify_signal();
> > > ...
> > > }
> >
> > I see, io_run_task_work() is too late, sk_stream_wait_memory() is
> > already woken up.
> >
> > I still think this patch is unsafe. What about explicitly handling the
> > restart in tls_sw_release_resources_tx() ? The main point is that such
> > function is called by inet_release() and the latter can't be re-
> > started.
>
> I don't think there's anything I can do in tls_sw_release_resources_tx().
> When entering this function TIF_NOTIFY_SIGNAL is not (yet) set. It gets
> set at some point while tls_sw_release_resources_tx() is running. I find
> it set when tls_tx_records() returns with -ERESTARTSYS. I tried clearing
> TIF_NOTIFY_SIGNAL then and called tls_tx_records() again, but that doesn't
> work.
Seems the discussion got stuck, what are the blocking points?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 2023-11-17 at 11:43 +01, Marc Kleine-Budde <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On 27.10.2023 14:04:32, Sascha Hauer wrote:
>> On Thu, Oct 26, 2023 at 10:49:18AM +0200, Paolo Abeni wrote:
>> > On Thu, 2023-10-26 at 09:03 +0200, Sascha Hauer wrote:
>> > > On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
>> > > > On Mon, 2023-10-23 at 14:13 +0200, 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().
>> > > > >
>> > > > > It seems signal_pending() is too broad, so this patch replaces it with
>> > > > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
>> > > >
>> > > > This looks dangerous, at best. Other possible legit users setting
>> > > > TIF_NOTIFY_SIGNAL will be broken.
>> > > >
>> > > > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
>> > >
>> > > I don't have an idea how io_run_task_work() comes into play here, but it
>> > > seems it already clears TIF_NOTIFY_SIGNAL:
>> > >
>> > > static inline int io_run_task_work(void)
>> > > {
>> > > /*
>> > > * Always check-and-clear the task_work notification signal. With how
>> > > * signaling works for task_work, we can find it set with nothing to
>> > > * run. We need to clear it for that case, like get_signal() does.
>> > > */
>> > > if (test_thread_flag(TIF_NOTIFY_SIGNAL))
>> > > clear_notify_signal();
>> > > ...
>> > > }
>> >
>> > I see, io_run_task_work() is too late, sk_stream_wait_memory() is
>> > already woken up.
>> >
>> > I still think this patch is unsafe. What about explicitly handling the
>> > restart in tls_sw_release_resources_tx() ? The main point is that such
>> > function is called by inet_release() and the latter can't be re-
>> > started.
>>
>> I don't think there's anything I can do in tls_sw_release_resources_tx().
>> When entering this function TIF_NOTIFY_SIGNAL is not (yet) set. It gets
>> set at some point while tls_sw_release_resources_tx() is running. I find
>> it set when tls_tx_records() returns with -ERESTARTSYS. I tried clearing
>> TIF_NOTIFY_SIGNAL then and called tls_tx_records() again, but that doesn't
>> work.
>
> Seems the discussion got stuck, what are the blocking points?
Ping!
Any pointers on how to get this sorted out?
Best regards,
Steffen
--
Pengutronix e.K. | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |
On Tue, 2023-12-19 at 12:00 +0100, Steffen Trumtrar wrote:
> On 2023-11-17 at 11:43 +01, Marc Kleine-Budde <[email protected]> wrote:
>
> > [[PGP Signed Part:Undecided]]
> > On 27.10.2023 14:04:32, Sascha Hauer wrote:
> > > On Thu, Oct 26, 2023 at 10:49:18AM +0200, Paolo Abeni wrote:
> > > > On Thu, 2023-10-26 at 09:03 +0200, Sascha Hauer wrote:
> > > > > On Tue, Oct 24, 2023 at 03:56:17PM +0200, Paolo Abeni wrote:
> > > > > > On Mon, 2023-10-23 at 14:13 +0200, 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().
> > > > > > >
> > > > > > > It seems signal_pending() is too broad, so this patch replaces it with
> > > > > > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
> > > > > >
> > > > > > This looks dangerous, at best. Other possible legit users setting
> > > > > > TIF_NOTIFY_SIGNAL will be broken.
> > > > > >
> > > > > > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work() ?
> > > > >
> > > > > I don't have an idea how io_run_task_work() comes into play here, but it
> > > > > seems it already clears TIF_NOTIFY_SIGNAL:
> > > > >
> > > > > static inline int io_run_task_work(void)
> > > > > {
> > > > > /*
> > > > > * Always check-and-clear the task_work notification signal. With how
> > > > > * signaling works for task_work, we can find it set with nothing to
> > > > > * run. We need to clear it for that case, like get_signal() does.
> > > > > */
> > > > > if (test_thread_flag(TIF_NOTIFY_SIGNAL))
> > > > > clear_notify_signal();
> > > > > ...
> > > > > }
> > > >
> > > > I see, io_run_task_work() is too late, sk_stream_wait_memory() is
> > > > already woken up.
> > > >
> > > > I still think this patch is unsafe. What about explicitly handling the
> > > > restart in tls_sw_release_resources_tx() ? The main point is that such
> > > > function is called by inet_release() and the latter can't be re-
> > > > started.
> > >
> > > I don't think there's anything I can do in tls_sw_release_resources_tx().
> > > When entering this function TIF_NOTIFY_SIGNAL is not (yet) set. It gets
> > > set at some point while tls_sw_release_resources_tx() is running. I find
> > > it set when tls_tx_records() returns with -ERESTARTSYS. I tried clearing
> > > TIF_NOTIFY_SIGNAL then and called tls_tx_records() again, but that doesn't
> > > work.
> >
> > Seems the discussion got stuck, what are the blocking points?
>
> Ping!
>
> Any pointers on how to get this sorted out?
I raised the point if this patch could be dangerous outside of the
specific scenario and I did not get any reply to that.
To be more explicit: why this will not cause user-space driven
connect() from missing relevant events? Why this is needed only here
and not in all the many others event loops waiting for signals? Why
can't the issue be addressed in any place more closely tied to the
scenario, e.g. io_uring or ktls?
Thanks
Paolo