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;
Date:   Mon, 17 May 2021 12:10:35 -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>,
        Linux Containers <containers@lists.linux.dev>,
        Andy Lutomirski <luto@kernel.org>,
        Rodrigo Campos <rodrigo@kinvolk.io>,
        Mauricio =?iso-8859-1?Q?V=E1squez?= Bernal 
        <mauricio@kinvolk.io>, Giuseppe Scrivano <gscrivan@redhat.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@linux.microsoft.com>
Subject: Re: [PATCH 3/4] seccomp: Support atomic "addfd + send reply"
Message-ID: <20210517181035.GH1964106@cisco>
References: <20210502001851.3346-1-sargun@sargun.me>
 <20210502001851.3346-4-sargun@sargun.me>
 <20210511215010.GB1964106@cisco>
 <CAMp4zn-crcdi6AR9VfAmpb0TO9sDtpdwA4C59ERqwdoQW7yTAg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMp4zn-crcdi6AR9VfAmpb0TO9sDtpdwA4C59ERqwdoQW7yTAg@mail.gmail.com>
Precedence: bulk

On Mon, May 17, 2021 at 10:53:55AM -0700, Sargun Dhillon wrote:
> On Tue, May 11, 2021 at 2:50 PM Tycho Andersen <tycho@tycho.pizza> wrote:
> > > The struct seccomp_notif_resp, used when doing SECCOMP_IOCTL_NOTIF_SEND
> > > ioctl() to send a response to the target, has three more fields that we
> > > don't allow to set when doing the addfd ioctl() to also return. The
> > > reasons to disallow each field are:
> > >  * val: This will be set to the new allocated fd. No point taking it
> > >    from userspace in this case.
> > >  * error: If this is non-zero, the value is ignored. Therefore,
> > >    it is pointless in this case as we want to return the value.
> > >  * flags: The only flag is to let userspace continue to execute the
> > >    syscall. This seems pointless, as we want the syscall to return the
> > >    allocated fd.
> > >
> > > This is why those fields are not possible to set when using this new
> > > flag.
> >
> > I don't quite understand this; you don't need a NOTIF_SEND at all
> > with the way this currently works, right?
> >
> I reworded:
> 
> This effectively combines SECCOMP_IOCTL_NOTIF_ADDFD and
> SECCOMP_IOCTL_NOTIF_SEND into an atomic opteration. The notification's
> return value, nor error can be set by the user. Upon successful invocation
> of the SECCOMP_IOCTL_NOTIF_ADDFD ioctl with the SECCOMP_ADDFD_FLAG_SEND
> flag, the notifying process's errno will be 0, and the return value will
> be the file descriptor number that was installed.
> 
> How does that sound?

Works for me, thanks!

> > > @@ -1113,7 +1136,7 @@ static int seccomp_do_user_notification(int this_syscall,
> > >                                                struct seccomp_kaddfd, list);
> > >               /* Check if we were woken up by a addfd message */
> > >               if (addfd)
> > > -                     seccomp_handle_addfd(addfd);
> > > +                     seccomp_handle_addfd(addfd, &n);
> > >
> > >       }  while (n.state != SECCOMP_NOTIFY_REPLIED);
> > >
> >
> > This while() bit is introduced in the previous patch, can we fold this
> > deletion into that somehow?
> I'm not sure what you're getting at. This just an argument change which
> also passes the notification to the addfd function. The patch is split out
> to allow it to be backported to stable.

Yeah, I was mis-reading, you can ignore this. Sorry for the noise.

If you send another version, you can call the series:

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

Tycho