Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210517193908.3113-1-sargun@sargun.me> <20210517193908.3113-3-sargun@sargun.me>
In-Reply-To: <20210517193908.3113-3-sargun@sargun.me>
From:   Rodrigo Campos <rodrigo@kinvolk.io>
Date:   Tue, 25 May 2021 18:03:38 +0200
Message-ID: <CACaBj2Y5YsrbFCw1m9U=8S8uJFiPo_c4riitDE5z-re65a-x9g@mail.gmail.com>
Subject: Re: [PATCH v2 2/4] seccomp: Refactor notification handler to prepare
 for new semantics
To:     Sargun Dhillon <sargun@sargun.me>
Cc:     Kees Cook <keescook@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>, containers@lists.linux.dev,
        Tycho Andersen <tycho@tycho.pizza>,
        Andy Lutomirski <luto@kernel.org>,
        =?UTF-8?Q?Mauricio_V=C3=A1squez_Bernal?= <mauricio@kinvolk.io>,
        Giuseppe Scrivano <gscrivan@redhat.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= <mic@linux.microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 17, 2021 at 9:39 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> This refactors the user notification code to have a do / while loop around
> the completion condition. This has a small change in semantic, in that
> previously we ignored addfd calls upon wakeup if the notification had been
> responded to, but instead with the new change we check for an outstanding
> addfd calls prior to returning to userspace.

I understand why this was a readability improvement on the old
patchset (that included the wait_killable semantics), as it completely
changed the loop. But now we only have the atomic addfd+send reply
that does minimal changes to this part (add a param to a function).

Is it worth changing the semantics?

> Rodrigo Campos also identified a bug that can result in addfd causing
> an early return, when the supervisor didn't actually handle the
> syscall [1].
>
> [1]: https://lore.kernel.org/lkml/20210413160151.3301-1-rodrigo@kinvolk.io/

I was about to resend this, but I'd like to know what others think.

I'm okay with applying any patches to solve the issue (mine linked
there or this one), slightly in favor of mine as the diff is way
simpler to backport (applies to 5.9+ kernels) and I don't see a reason
to change semantics. But no strong opinion.

Opinions?


Best,
Rodrigo