| Message ID | 20231023121346.4098160-1-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Do not break out of sk_stream_wait_memory() with TIF_NOTIFY_SIGNAL | expand |
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
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
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
On 2023-11-17 at 11:43 +01, Marc Kleine-Budde <mkl@pengutronix.de> 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 <mkl@pengutronix.de> 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
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)
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") Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- net/core/stream.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)