Re: [Qemu-devel] [PATCH] libseccomp: add cacheflush to whitelist

[Andrew Jones <drjones@redhat.com>]

On Wed, Oct 14, 2015 at 11:03:37AM +0200, Eduardo Otubo wrote:
> On Tue, Sep 29, 2015 at 11=39=46AM +0200, Eduardo Otubo wrote:
> > On Thu, Sep 24, 2015 at 03=50=04PM +0200, Andrew Jones wrote:
> > > On Thu, Sep 24, 2015 at 11:31:19AM +0200, Eduardo Otubo wrote:
> > > > On Wed, Jul 01, 2015 at 09=12=33AM -0400, Andrew Jones wrote:
> > > > > cacheflush is an arm-specific syscall that qemu built for arm
> > > > > uses. Add it to the whitelist.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > I'm not sure about the priority selection. Maybe cacheflush gets
> > > > > used frequently enough that it deserves a higher one?
> > > > 
> > > > The frequency is measured using strace and comparing the frequency they
> > > > appear among other syscalls. Can you run this analysis and double check
> > > > if the lowest priority is still accurate?
> > > 
> > > Hi Eduardo,
> > > 
> > > Short answer: The lowest priority is definitely correct.
> > > 
> > > Long answer:
> > > 
> > > I ran strace while installing a new guest, of 3.6 million syscalls,
> > > only 5 were cacheflush. Of course the syscalls used (and their frequency)
> > > is host-type, qemu machine-type, config (qemu command line), and guest
> > > workload specific. So, ideally, qemu machine-types would register their
> > > own whitelists, possibly modified by host-type. For example, I ran the
> > > mach-virt machine-type on both a midway and a mustang. In both cases it
> > > was a basic guest config and an install-type workload. For the mustang,
> > > over 55% of the syscalls were ioctl, but, for the midway, ioctls were
> > > 16% and 43% were clock_gettime. I generated a most-used-first list for
> > > each. Neither list really matched up well with seccomp_whitelist (except
> > > for futex).
> > > 
> > > Besides allowing machine types to help set priorities, it may also be
> > > nice if both compile-time and run-time configs could further reduce the
> > > whitelist. For example, mlockall is only necessary if '-realtime mlock=on'
> > > is passed on the command line.
> > > 
> > > Thanks,
> > > drew
> > > 
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > > 
> > > > > This patch isn't really necessary yet due to ae6e8ef11e6c: "Revert
> > > > > seccomp tests that allow it to be used on non-x86 architectures",
> > > > > which we can't revert until libseccomp has released a fix for
> > > > > arm-specific syscall symbol naming, but when linking to a patched
> > > > > libseccomp and reverting ae6e8ef11e6c, then this patch allows
> > > > > guests to boot with '-sandbox on'.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  qemu-seccomp.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > > > > index f9de0d3390feb..33644a4e3c3d3 100644
> > > > > --- a/qemu-seccomp.c
> > > > > +++ b/qemu-seccomp.c
> > > > > @@ -237,7 +237,8 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
> > > > >      { SCMP_SYS(fadvise64), 240 },
> > > > >      { SCMP_SYS(inotify_init1), 240 },
> > > > >      { SCMP_SYS(inotify_add_watch), 240 },
> > > > > -    { SCMP_SYS(mbind), 240 }
> > > > > +    { SCMP_SYS(mbind), 240 },
> > > > > +    { SCMP_SYS(cacheflush), 240 },
> 
> FYI: I had to fixed this minor mistake (using comma at the end of the
> list) before applying your patch.

Does that violate a QEMU coding style? Because I actually put it there
on purpose so the next addition wouldn't have to modify the line (by
adding a comma), making git-blame more difficult to use.

Thanks,
drew

> 
> -- 
> Eduardo Otubo
> ProfitBricks GmbH