Re: [Qemu-devel] [PATCH] seccomp: report more useful errors from seccomp

[Daniel P . Berrangé]

On Mon, Mar 25, 2019 at 04:25:19PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 25, 2019 at 3:07 PM Daniel P. Berrangé <address@hidden> wrote:
> >
> > Most of the seccomp functions return errnos as a negative return
> > value. The code is currently ignoring these and reporting a generic
> > error message for all seccomp failure scenarios making debugging
> > painful. Report a more precise error from each failed call and include
> > errno if it is available.
> >
> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> 
> Is this for 4.0? Eligible imho.

I don't really mind either way.

> 
> Reviewed-by: Marc-André Lureau <address@hidden>
> 
> > ---
> >  qemu-seccomp.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > index 36d5829831..8daa9e0528 100644
> > --- a/qemu-seccomp.c
> > +++ b/qemu-seccomp.c
> > @@ -138,21 +138,23 @@ static uint32_t qemu_seccomp_get_kill_action(void)
> >  }
> >
> >
> > -static int seccomp_start(uint32_t seccomp_opts)
> > +static int seccomp_start(uint32_t seccomp_opts, Error **errp)
> >  {
> > -    int rc = 0;
> > +    int rc = -1;
> >      unsigned int i = 0;
> >      scmp_filter_ctx ctx;
> >      uint32_t action = qemu_seccomp_get_kill_action();
> >
> >      ctx = seccomp_init(SCMP_ACT_ALLOW);
> >      if (ctx == NULL) {
> > -        rc = -1;
> > +        error_setg(errp, "failed to initialize seccomp context");
> >          goto seccomp_return;
> >      }
> >
> >      rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1);
> >      if (rc != 0) {
> > +        error_setg_errno(errp, -rc,
> > +                         "failed to set seccomp thread synchronization");
> >          goto seccomp_return;
> >      }
> >
> > @@ -164,15 +166,21 @@ static int seccomp_start(uint32_t seccomp_opts)
> >          rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
> >                                      blacklist[i].narg, 
> > blacklist[i].arg_cmp);
> >          if (rc < 0) {
> > +            error_setg_errno(errp, -rc,
> > +                             "failed to add seccomp blacklist rules");
> >              goto seccomp_return;
> >          }
> >      }
> >
> >      rc = seccomp_load(ctx);
> > +    if (rc < 0) {
> > +        error_setg_errno(errp, -rc,
> > +                         "failed to load seccomp syscall filter in 
> > kernel");
> > +    }
> >
> >    seccomp_return:
> >      seccomp_release(ctx);
> > -    return rc;
> > +    return rc < 0 ? -1 : 0;
> >  }
> >
> >  #ifdef CONFIG_SECCOMP
> > @@ -242,9 +250,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error 
> > **errp)
> >              }
> >          }
> >
> > -        if (seccomp_start(seccomp_opts) < 0) {
> > -            error_setg(errp, "failed to install seccomp syscall filter "
> > -                       "in the kernel");
> > +        if (seccomp_start(seccomp_opts, errp) < 0) {
> >              return -1;
> >          }
> >      }
> > --
> > 2.20.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|