Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp user notifier

From: Rodrigo Campos <rodrigo@kinvolk.io>
To: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Containers <containers@lists.linux-foundation.org>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Will Drewry <wad@chromium.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Alban Crequy <alban@kinvolk.io>
Subject: Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp user notifier
Date: Tue, 3 May 2022 16:27:18 +0200	[thread overview]
Message-ID: <CACaBj2ba08sQTvJSTuApiXAcsRsC09UMbA3AN744r6FEngBGow@mail.gmail.com> (raw)
In-Reply-To: <20220502160415.GA1289934@ircssh-3.c.rugged-nimbus-611.internal>

On Mon, May 2, 2022 at 6:04 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> On Mon, May 02, 2022 at 04:15:07PM +0200, Rodrigo Campos wrote:
> > (I couldn't git-am this locally, so maybe I'm injecting this at the
> > wrong parts mentally when looking at the other code for more context.
> > Sorry if that is the case :))
> >
> > Why do we need to complete() only in this error path? As far as I can
> > see this is on the error path where the copy to userspace failed and
> > we want to reset this notification.
> This error path acts as follows
> (Say, S: Supervisor, P: Notifying Process, U: User)
>
> P: 2 <-- Pid
> P: getppid() // Generated notification
> P: Waiting in wait_interruptible state
> S: Calls receive notification, and the codepath gets up to the poin
>    where it's copying the notification to userspace
> U: kill -SIGURG 2 // Send a kill signal to the notifying process
> P: Waiting in the wait_killable state
> S: Kernel fails to copy notification into user memory, and resets
>    the notification and returns an error
>
> If we do not have the reset, the P will never return to wait interruptible.

Ohhh, because we want the wait to be interruptible again! Right, I
forgot we should reset to that state again, until the notification is
indeed handled.

What if we say something along those lines in the comment, then? Like:

// Make the other side go back to wait interruptible, the notification
is not SENT.

Something like that would at least help me in the future :)

> > We _need_ to call complete() in the non error path here so the other
> > side wakes up and switches to a killable wait. As we are not doing
> > this (for the non error path), this will basically not achieve a
> > wait_killable() at all.
> >
> No, because here, we check if we were waiting interruptible, and
> then we switch to wait_killable:

Ohhh, right right right. This is lazily changing to wait killable only
when something already wakes up the process. Sorry, I overlooked that.

Best,
Rodrigo