Received-SPF: pass (google.com: domain of linux-kernel+bounces-5244-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <e1e15554bfa5cfc8048d6074eedbc83c4d912c98.camel@redhat.com>
Subject: Re: [PATCH] net: Do not break out of sk_stream_wait_memory() with
 TIF_NOTIFY_SIGNAL
From: Paolo Abeni <pabeni@redhat.com>
To: Steffen Trumtrar <s.trumtrar@pengutronix.de>, Marc Kleine-Budde
	 <mkl@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>, Jens Axboe <axboe@kernel.dk>, 
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "David S . Miller"
 <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
 kernel@pengutronix.de
Date: Tue, 19 Dec 2023 14:13:16 +0100
In-Reply-To: <87cyv2wj4e.fsf@pengutronix.de>
References: <20231023121346.4098160-1-s.hauer@pengutronix.de>
	 <addf492843338e853f7fda683ce35050f26c9da0.camel@redhat.com>
	 <20231026070310.GY3359458@pengutronix.de>
	 <8404022493c5ceda74807a3407e5a087425678e2.camel@redhat.com>
	 <20231027120432.GB3359458@pengutronix.de>
	 <20231117-starter-unvisited-d10f0314ae76-mkl@pengutronix.de>
	 <87cyv2wj4e.fsf@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
Precedence: bulk
MIME-Version: 1.0

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:
>=20
> > [[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_memo=
ry() 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_ad=
d() by
> > > > > > > calling task_work_add().
> > > > > > >=20
> > > > > > > It seems signal_pending() is too broad, so this patch replace=
s it with
> > > > > > > task_sigpending(), thus ignoring the TIF_NOTIFY_SIGNAL flag.
> > > > > >=20
> > > > > > This looks dangerous, at best. Other possible legit users setti=
ng
> > > > > > TIF_NOTIFY_SIGNAL will be broken.
> > > > > >=20
> > > > > > Can't you instead clear TIF_NOTIFY_SIGNAL in io_run_task_work()=
 ?
> > > > >=20
> > > > > I don't have an idea how io_run_task_work() comes into play here,=
 but it
> > > > > seems it already clears TIF_NOTIFY_SIGNAL:
> > > > >=20
> > > > > static inline int io_run_task_work(void)
> > > > > {
> > > > >         /*
> > > > >          * Always check-and-clear the task_work notification sign=
al. 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_signa=
l() does.
> > > > >          */
> > > > >         if (test_thread_flag(TIF_NOTIFY_SIGNAL))
> > > > >                 clear_notify_signal();
> > > > > 	...
> > > > > }
> > > >=20
> > > > I see, io_run_task_work() is too late, sk_stream_wait_memory() is
> > > > already woken up.
> > > >=20
> > > > I still think this patch is unsafe. What about explicitly handling =
the
> > > > restart in tls_sw_release_resources_tx() ? The main point is that s=
uch
> > > > function is called by inet_release() and the latter can't be re-
> > > > started.
> > >=20
> > > I don't think there's anything I can do in tls_sw_release_resources_t=
x().
> > > When entering this function TIF_NOTIFY_SIGNAL is not (yet) set. It ge=
ts
> > > set at some point while tls_sw_release_resources_tx() is running. I f=
ind
> > > it set when tls_tx_records() returns with -ERESTARTSYS. I tried clear=
ing
> > > TIF_NOTIFY_SIGNAL then and called tls_tx_records() again, but that do=
esn't
> > > work.
> >=20
> > Seems the discussion got stuck, what are the blocking points?
>=20
> Ping!
>=20
> 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