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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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