[Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Eric Ren]

SCMP_FLTATTR_CTL_TSYNC flag is only available on
Linux Kernel 3.17 or greater. So, conditional compile
to make virtio-fs work on older host kernel.

Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
---
 contrib/virtiofsd/seccomp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
index 4e388adc9c..5a28a90859 100644
--- a/contrib/virtiofsd/seccomp.c
+++ b/contrib/virtiofsd/seccomp.c
@@ -11,6 +11,7 @@
 #include <errno.h>
 #include <seccomp.h>
 #include <glib.h>
+#include <linux/version.h>
 #include "seccomp.h"
 
 static const int syscall_whitelist[] = {
@@ -92,9 +93,12 @@ void setup_seccomp(void)
 		err(1, "seccomp_init()");
 	}
 
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)
+	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
 	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
 		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");
 	}
+#endif
 
 	for (i = 0; i < G_N_ELEMENTS(syscall_whitelist); i++) {
 		if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
-- 
2.17.2 (Apple Git-113)

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Dr. David Alan Gilbert]

* Eric Ren (renzhen@linux.alibaba.com) wrote:
> SCMP_FLTATTR_CTL_TSYNC flag is only available on
> Linux Kernel 3.17 or greater. So, conditional compile
> to make virtio-fs work on older host kernel.
> 
> Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> ---
>  contrib/virtiofsd/seccomp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
> index 4e388adc9c..5a28a90859 100644
> --- a/contrib/virtiofsd/seccomp.c
> +++ b/contrib/virtiofsd/seccomp.c
> @@ -11,6 +11,7 @@
>  #include <errno.h>
>  #include <seccomp.h>
>  #include <glib.h>
> +#include <linux/version.h>
>  #include "seccomp.h"
>  
>  static const int syscall_whitelist[] = {
> @@ -92,9 +93,12 @@ void setup_seccomp(void)
>  		err(1, "seccomp_init()");
>  	}
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)

I don't really like using kernel versions, because sometimes downstreams
backport stuff (I checked and it looks like RHEL7 did this somewhere
around 7.5).

If I understand correctly the right thing to do is check the
SCMP_VER_MAJOR/MINOR/MICRO version defines; and I think tsync came in
with 2.3.1.

> +	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
>  	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
>  		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");

Also, what happens if this fails?  e.g. I run it on an older kernel than
it's built for;  do we actually fail here or just print the error.

Eithe rway, is it actually safe without this define - or does the thread
which actually runs the work not get the support?

Dave

>  	}
> +#endif
>  
>  	for (i = 0; i < G_N_ELEMENTS(syscall_whitelist); i++) {
>  		if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
> -- 
> 2.17.2 (Apple Git-113)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Eric Ren]

On Thu, Jul 11, 2019 at 12:25:58PM +0100, Dr. David Alan Gilbert wrote:
> * Eric Ren (renzhen@linux.alibaba.com) wrote:
> > SCMP_FLTATTR_CTL_TSYNC flag is only available on
> > Linux Kernel 3.17 or greater. So, conditional compile
> > to make virtio-fs work on older host kernel.
> > 
> > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > ---
> >  contrib/virtiofsd/seccomp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
> > index 4e388adc9c..5a28a90859 100644
> > --- a/contrib/virtiofsd/seccomp.c
> > +++ b/contrib/virtiofsd/seccomp.c
> > @@ -11,6 +11,7 @@
> >  #include <errno.h>
> >  #include <seccomp.h>
> >  #include <glib.h>
> > +#include <linux/version.h>
> >  #include "seccomp.h"
> >  
> >  static const int syscall_whitelist[] = {
> > @@ -92,9 +93,12 @@ void setup_seccomp(void)
> >  		err(1, "seccomp_init()");
> >  	}
> >  
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)
> 
> I don't really like using kernel versions, because sometimes downstreams
> backport stuff (I checked and it looks like RHEL7 did this somewhere
> around 7.5).
> 
> If I understand correctly the right thing to do is check the
> SCMP_VER_MAJOR/MINOR/MICRO version defines; and I think tsync came in
> with 2.3.1.
> 
> > +	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
> >  	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
> >  		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");
> 
> Also, what happens if this fails?  e.g. I run it on an older kernel than
> it's built for;  do we actually fail here or just print the error.

So the result also applies if checking SCMP_VER_MAJOR/MINOR/MICRO
defines :-/

> 
> Eithe rway, is it actually safe without this define - 
Actually I don't know the exact effect of SCMP_FLTATR_CTL_TSYNC attr.
What if we gives a warning instead of error if failing to set it?

> or does the thread
> which actually runs the work not get the support?

Sorry, I fail to get your point here?

Regards,
Eric

> 
> Dave
> 
> >  	}
> > +#endif
> >  
> >  	for (i = 0; i < G_N_ELEMENTS(syscall_whitelist); i++) {
> >  		if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
> > -- 
> > 2.17.2 (Apple Git-113)
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Dr. David Alan Gilbert]

* Eric Ren (renzhen@linux.alibaba.com) wrote:
> On Thu, Jul 11, 2019 at 12:25:58PM +0100, Dr. David Alan Gilbert wrote:
> > * Eric Ren (renzhen@linux.alibaba.com) wrote:
> > > SCMP_FLTATTR_CTL_TSYNC flag is only available on
> > > Linux Kernel 3.17 or greater. So, conditional compile
> > > to make virtio-fs work on older host kernel.
> > > 
> > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > > ---
> > >  contrib/virtiofsd/seccomp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
> > > index 4e388adc9c..5a28a90859 100644
> > > --- a/contrib/virtiofsd/seccomp.c
> > > +++ b/contrib/virtiofsd/seccomp.c
> > > @@ -11,6 +11,7 @@
> > >  #include <errno.h>
> > >  #include <seccomp.h>
> > >  #include <glib.h>
> > > +#include <linux/version.h>
> > >  #include "seccomp.h"
> > >  
> > >  static const int syscall_whitelist[] = {
> > > @@ -92,9 +93,12 @@ void setup_seccomp(void)
> > >  		err(1, "seccomp_init()");
> > >  	}
> > >  
> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)
> > 
> > I don't really like using kernel versions, because sometimes downstreams
> > backport stuff (I checked and it looks like RHEL7 did this somewhere
> > around 7.5).
> > 
> > If I understand correctly the right thing to do is check the
> > SCMP_VER_MAJOR/MINOR/MICRO version defines; and I think tsync came in
> > with 2.3.1.
> > 
> > > +	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
> > >  	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
> > >  		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");
> > 
> > Also, what happens if this fails?  e.g. I run it on an older kernel than
> > it's built for;  do we actually fail here or just print the error.
> 
> So the result also applies if checking SCMP_VER_MAJOR/MINOR/MICRO
> defines :-/
> 
> > 
> > Eithe rway, is it actually safe without this define - 
> Actually I don't know the exact effect of SCMP_FLTATR_CTL_TSYNC attr.
> What if we gives a warning instead of error if failing to set it?
> 
> > or does the thread
> > which actually runs the work not get the support?
> 
> Sorry, I fail to get your point here?

I don't know seccomp that well (lets ask Stefan!), but my understanding
of TSYNC is that it causes all threads to get the new seccomp rules
not just the thread we're running in.  So I'm worried that if we don't
have TSYNC, some threads will run without the protection they need.

Dave

> Regards,
> Eric
> 
> > 
> > Dave
> > 
> > >  	}
> > > +#endif
> > >  
> > >  	for (i = 0; i < G_N_ELEMENTS(syscall_whitelist); i++) {
> > >  		if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
> > > -- 
> > > 2.17.2 (Apple Git-113)
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Eric Ren]

Hi,

On Thu, Jul 11, 2019 at 01:32:54PM +0100, Dr. David Alan Gilbert wrote:
> * Eric Ren (renzhen@linux.alibaba.com) wrote:
[...]
> > > Eithe rway, is it actually safe without this define - 
> > Actually I don't know the exact effect of SCMP_FLTATR_CTL_TSYNC attr.
> > What if we gives a warning instead of error if failing to set it?
> > 
> > > or does the thread
> > > which actually runs the work not get the support?
> > 
> > Sorry, I fail to get your point here?
> 
> I don't know seccomp that well (lets ask Stefan!), but my understanding
> of TSYNC is that it causes all threads to get the new seccomp rules
> not just the thread we're running in.  So I'm worried that if we don't
> have TSYNC, some threads will run without the protection they need.

Thanks, I get it. On older kernel host, it has not this support so
I guess a warning message is the best we can do ;-)

Regards,
Eric

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Stefan Hajnoczi]

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

On Wed, Jul 10, 2019 at 09:06:42PM +0800, Eric Ren wrote:
> SCMP_FLTATTR_CTL_TSYNC flag is only available on
> Linux Kernel 3.17 or greater. So, conditional compile
> to make virtio-fs work on older host kernel.
> 
> Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> ---
>  contrib/virtiofsd/seccomp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
> index 4e388adc9c..5a28a90859 100644
> --- a/contrib/virtiofsd/seccomp.c
> +++ b/contrib/virtiofsd/seccomp.c
> @@ -11,6 +11,7 @@
>  #include <errno.h>
>  #include <seccomp.h>
>  #include <glib.h>
> +#include <linux/version.h>
>  #include "seccomp.h"
>  
>  static const int syscall_whitelist[] = {
> @@ -92,9 +93,12 @@ void setup_seccomp(void)
>  		err(1, "seccomp_init()");
>  	}
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)
> +	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
>  	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
>  		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");
>  	}
> +#endif

There are no threads when setup_seccomp() is invoked so the TSYNC
attribute is not required.

Please remove the TSYNC call.  Then we don't need to worry about version
or feature checks.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH] virtiofsd: conditional compile seccomp flag support

[Eric Ren]

On Fri, Jul 12, 2019 at 10:04:46AM +0200, Stefan Hajnoczi wrote:
> On Wed, Jul 10, 2019 at 09:06:42PM +0800, Eric Ren wrote:
> > SCMP_FLTATTR_CTL_TSYNC flag is only available on
> > Linux Kernel 3.17 or greater. So, conditional compile
> > to make virtio-fs work on older host kernel.
> > 
> > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > ---
> >  contrib/virtiofsd/seccomp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
> > index 4e388adc9c..5a28a90859 100644
> > --- a/contrib/virtiofsd/seccomp.c
> > +++ b/contrib/virtiofsd/seccomp.c
> > @@ -11,6 +11,7 @@
> >  #include <errno.h>
> >  #include <seccomp.h>
> >  #include <glib.h>
> > +#include <linux/version.h>
> >  #include "seccomp.h"
> >  
> >  static const int syscall_whitelist[] = {
> > @@ -92,9 +93,12 @@ void setup_seccomp(void)
> >  		err(1, "seccomp_init()");
> >  	}
> >  
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)
> > +	// SCMP_FLTATR_CTL_TSYNC flag is only available on Linux Kernel 3.17 or greater
> >  	if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1) != 0) {
> >  		err(1, "seccomp_attr_set(ctx, SCMP_FLTATTR_CTL_TSYNC, 1)");
> >  	}
> > +#endif
> 
> There are no threads when setup_seccomp() is invoked so the TSYNC
> attribute is not required.

Aha, great.

> Please remove the TSYNC call.  Then we don't need to worry about version
> or feature checks.

OK.

Regards,
Eric


> 
> Thanks,
> Stefan