Re: [PATCH 3/4] seccomp: Support atomic "addfd + send reply"

[Tycho Andersen <tycho@tycho.pizza>]

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