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

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

[Andrew Jones]

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?

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 },
 };
 
 int seccomp_start(void)
-- 
2.1.0

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

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

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?

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 },
>  };
>  
>  int seccomp_start(void)
> -- 
> 2.1.0
> 

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andrew Jones]

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 },
> >  };
> >  
> >  int seccomp_start(void)
> > -- 
> > 2.1.0
> > 
> 
> -- 
> Eduardo Otubo
> ProfitBricks GmbH

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

[Peter Maydell]

On 24 September 2015 at 06:50, Andrew Jones <drjones@redhat.com> wrote:
> 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).

This is strongly dependent on host CPU and what you're doing.
If you build QEMU for 64-bit ARM then we should be able to do
the cache ops from userspace without syscalls. If you're running
KVM then we probably only flush the cache a few times at startup
when we load the ROM images and so on. If you're using TCG
emulation I would expect that we do quite a lot of cache ops...

You could reasonably argue that anybody who cares about seccomp
is going to be running with KVM, of course.

thanks
-- PMM

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

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

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 },
> > >  };
> > >  
> > >  int seccomp_start(void)
> > > -- 
> > > 2.1.0
> > > 

Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]

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.

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andrew Jones]

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

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

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> 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:
[...]
>> > > > > 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.

We have trailing commas all over the place, for precisely the reason you
gave.  I recommend to put it right back.

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

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

On Wed, Oct 14, 2015 at 03=25=00PM +0200, Markus Armbruster wrote:
> Andrew Jones <drjones@redhat.com> writes:
> 
> > 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:
> [...]
> >> > > > > 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.
> 
> We have trailing commas all over the place, for precisely the reason you
> gave.  I recommend to put it right back.
> 

Well, that makes a lot of sense. I'll just put it back again and send
the pull request once more.

ps.: I just thought the guideline said to make sure every commit should
be "compilable" with no errors, perhaps I saw it somewhere else. Sorry
for the trouble.

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andrew Jones]

On Wed, Oct 14, 2015 at 04:58:13PM +0200, Eduardo Otubo wrote:
> On Wed, Oct 14, 2015 at 03=25=00PM +0200, Markus Armbruster wrote:
> > Andrew Jones <drjones@redhat.com> writes:
> > 
> > > 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:
> > [...]
> > >> > > > > 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.
> > 
> > We have trailing commas all over the place, for precisely the reason you
> > gave.  I recommend to put it right back.
> > 
> 
> Well, that makes a lot of sense. I'll just put it back again and send
> the pull request once more.
> 
> ps.: I just thought the guideline said to make sure every commit should
> be "compilable" with no errors, perhaps I saw it somewhere else. Sorry
> for the trouble.

Does your complier complain about that trailing comma? I thought
allowing that was even part of the ANSI C standard.

Thanks,
drew

> 
> -- 
> Eduardo Otubo
> ProfitBricks GmbH

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

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> On Wed, Oct 14, 2015 at 04:58:13PM +0200, Eduardo Otubo wrote:
>> On Wed, Oct 14, 2015 at 03=25=00PM +0200, Markus Armbruster wrote:
>> > Andrew Jones <drjones@redhat.com> writes:
>> > 
>> > > 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:
>> > [...]
>> > >> > > > > 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.
>> > 
>> > We have trailing commas all over the place, for precisely the reason you
>> > gave.  I recommend to put it right back.
>> > 
>> 
>> Well, that makes a lot of sense. I'll just put it back again and send
>> the pull request once more.
>> 
>> ps.: I just thought the guideline said to make sure every commit should
>> be "compilable" with no errors, perhaps I saw it somewhere else. Sorry
>> for the trouble.
>
> Does your complier complain about that trailing comma? I thought
> allowing that was even part of the ANSI C standard.

It's C99.