Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 28 Apr 2022 13:34:25 -0600
From:   Tycho Andersen <tycho@tycho.pizza>
To:     Sargun Dhillon <sargun@sargun.me>
Cc:     Kees Cook <keescook@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 2/2] selftests/seccomp: Ensure that notifications come in
 FIFO order
Message-ID: <YmrsQZ2lNGHjGK6i@cisco>
References: <20220428015447.13661-1-sargun@sargun.me>
 <20220428015447.13661-2-sargun@sargun.me>
 <YmqTh68UkjVsTnUX@cisco>
 <CAMp4zn-725wHy2su_Dz8pEdzUv7tG=gQ+9=7hj5PXfZpQeOLjQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMp4zn-725wHy2su_Dz8pEdzUv7tG=gQ+9=7hj5PXfZpQeOLjQ@mail.gmail.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 09:38:10AM -0700, Sargun Dhillon wrote:
> On Thu, Apr 28, 2022 at 6:15 AM Tycho Andersen <tycho@tycho.pizza> wrote:
> > > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > > +             pid = fork();
> > > +             if (pid == 0) {
> > > +                     ret = syscall(__NR_getppid);
> > > +                     exit(ret != USER_NOTIF_MAGIC);
> > > +             }
> > > +             pids[i] = pid;
> > > +     }
> > > +
> > > +     /* This spins until all of the children are sleeping */
> > > +restart_wait:
> > > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > > +             if (get_proc_stat(pids[i]) != 'S') {
> > > +                     nanosleep(&delay, NULL);
> > > +                     goto restart_wait;
> > > +             }
> > > +     }
> >
> > I wonder if we should/can combine this loop with the previous one, and
> > wait for the child to sleep in getppid() before we fork the next one.
> > Otherwise isn't racy in the case that your loop continues to the next
> > iteration before the child processes are scheduled, so things might be
> > out of order? Maybe I'm missing something.
> >
> > In any case, this change seems reasonable to me.
> >
> > Tycho
> It's okay if the child processes are started out of order. The test just
> verifies that the calls are delivered in FIFO order according to when
> the syscall was called (not when the process was started), and we do
> this by just looking at the notification ID. It doesn't care about which
> process generated the notification.

I totally missed that you don't this, I just assumed you did. Thanks.

Anyway, you can add:

Acked-by: Tycho Andersen <tycho@tycho.pizza>

to both patches.

Tycho