Messages in this thread | | | From | Rodrigo Campos <> | Date | Tue, 3 May 2022 16:27:18 +0200 | Subject | Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp user notifier |
| |
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
| |