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

[Paolo Abeni]

On Tue, 2023-12-19 at 12:00 +0100, Steffen Trumtrar wrote:
> On 2023-11-17 at 11:43 +01, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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