[PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list

Vivek Goyal posted 13 patches 4 years, 4 months ago
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
[PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Vivek Goyal 4 years, 4 months ago
g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
syscall. Now these patches are making use of g_usleep(). So add
clock_nanosleep() to list of allowed syscalls.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_seccomp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index cd24b40b78..03080806c0 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
     SCMP_SYS(writev),
     SCMP_SYS(umask),
     SCMP_SYS(nanosleep),
+    SCMP_SYS(clock_nanosleep),
 };
 
 /* Syscalls used when --syslog is enabled */
-- 
2.31.1


Re: [PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Stefan Hajnoczi 4 years, 4 months ago
On Thu, Sep 30, 2021 at 11:30:37AM -0400, Vivek Goyal wrote:
> g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
> syscall. Now these patches are making use of g_usleep(). So add
> clock_nanosleep() to list of allowed syscalls.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_seccomp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> index cd24b40b78..03080806c0 100644
> --- a/tools/virtiofsd/passthrough_seccomp.c
> +++ b/tools/virtiofsd/passthrough_seccomp.c
> @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
>      SCMP_SYS(writev),
>      SCMP_SYS(umask),
>      SCMP_SYS(nanosleep),
> +    SCMP_SYS(clock_nanosleep),

This patch can be dropped once sleep has been replaced by a condvar.
Re: [Virtio-fs] [PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Vivek Goyal 4 years, 4 months ago
On Tue, Oct 05, 2021 at 01:22:58PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 30, 2021 at 11:30:37AM -0400, Vivek Goyal wrote:
> > g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
> > syscall. Now these patches are making use of g_usleep(). So add
> > clock_nanosleep() to list of allowed syscalls.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/passthrough_seccomp.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> > index cd24b40b78..03080806c0 100644
> > --- a/tools/virtiofsd/passthrough_seccomp.c
> > +++ b/tools/virtiofsd/passthrough_seccomp.c
> > @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
> >      SCMP_SYS(writev),
> >      SCMP_SYS(umask),
> >      SCMP_SYS(nanosleep),
> > +    SCMP_SYS(clock_nanosleep),
> 
> This patch can be dropped once sleep has been replaced by a condvar.

There is another sleep in do_pool_destroy() where we are waiting
for all current threads to exit.

do_pool_destroy() {
    g_usleep(10000);
}

Vivek


Re: [Virtio-fs] [PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Stefan Hajnoczi 4 years, 4 months ago
On Tue, Oct 05, 2021 at 11:16:18AM -0400, Vivek Goyal wrote:
> On Tue, Oct 05, 2021 at 01:22:58PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 30, 2021 at 11:30:37AM -0400, Vivek Goyal wrote:
> > > g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
> > > syscall. Now these patches are making use of g_usleep(). So add
> > > clock_nanosleep() to list of allowed syscalls.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_seccomp.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> > > index cd24b40b78..03080806c0 100644
> > > --- a/tools/virtiofsd/passthrough_seccomp.c
> > > +++ b/tools/virtiofsd/passthrough_seccomp.c
> > > @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
> > >      SCMP_SYS(writev),
> > >      SCMP_SYS(umask),
> > >      SCMP_SYS(nanosleep),
> > > +    SCMP_SYS(clock_nanosleep),
> > 
> > This patch can be dropped once sleep has been replaced by a condvar.
> 
> There is another sleep in do_pool_destroy() where we are waiting
> for all current threads to exit.
> 
> do_pool_destroy() {
>     g_usleep(10000);
> }

That won't be necessary if there's a way to avoid the thread pool :).
See my other reply about closing the OFD instead of using signals to
cancel blocking fcntl(2).

Stefan
Re: [Virtio-fs] [PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Vivek Goyal 4 years, 4 months ago
On Tue, Oct 05, 2021 at 04:50:43PM +0100, Stefan Hajnoczi wrote:
> On Tue, Oct 05, 2021 at 11:16:18AM -0400, Vivek Goyal wrote:
> > On Tue, Oct 05, 2021 at 01:22:58PM +0100, Stefan Hajnoczi wrote:
> > > On Thu, Sep 30, 2021 at 11:30:37AM -0400, Vivek Goyal wrote:
> > > > g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
> > > > syscall. Now these patches are making use of g_usleep(). So add
> > > > clock_nanosleep() to list of allowed syscalls.
> > > > 
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > >  tools/virtiofsd/passthrough_seccomp.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> > > > index cd24b40b78..03080806c0 100644
> > > > --- a/tools/virtiofsd/passthrough_seccomp.c
> > > > +++ b/tools/virtiofsd/passthrough_seccomp.c
> > > > @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
> > > >      SCMP_SYS(writev),
> > > >      SCMP_SYS(umask),
> > > >      SCMP_SYS(nanosleep),
> > > > +    SCMP_SYS(clock_nanosleep),
> > > 
> > > This patch can be dropped once sleep has been replaced by a condvar.
> > 
> > There is another sleep in do_pool_destroy() where we are waiting
> > for all current threads to exit.
> > 
> > do_pool_destroy() {
> >     g_usleep(10000);
> > }
> 
> That won't be necessary if there's a way to avoid the thread pool :).
> See my other reply about closing the OFD instead of using signals to
> cancel blocking fcntl(2).

Hi Stefan,

I responded to that email already. man fnctl does not say anything
about closing fd will unblock the waiter with -EINTR and I had a 
quick look at kernel code and did not find anything which suggested
closing fd will unblock current waiters.

So is this something you know works or you want me to try and see
if it works?

Thanks
Vivek


Re: [Virtio-fs] [PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
Posted by Stefan Hajnoczi 4 years, 4 months ago
On Tue, Oct 05, 2021 at 01:28:21PM -0400, Vivek Goyal wrote:
> On Tue, Oct 05, 2021 at 04:50:43PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Oct 05, 2021 at 11:16:18AM -0400, Vivek Goyal wrote:
> > > On Tue, Oct 05, 2021 at 01:22:58PM +0100, Stefan Hajnoczi wrote:
> > > > On Thu, Sep 30, 2021 at 11:30:37AM -0400, Vivek Goyal wrote:
> > > > > g_usleep() calls nanosleep() and that now seems to call clock_nanosleep()
> > > > > syscall. Now these patches are making use of g_usleep(). So add
> > > > > clock_nanosleep() to list of allowed syscalls.
> > > > > 
> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > > ---
> > > > >  tools/virtiofsd/passthrough_seccomp.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> > > > > index cd24b40b78..03080806c0 100644
> > > > > --- a/tools/virtiofsd/passthrough_seccomp.c
> > > > > +++ b/tools/virtiofsd/passthrough_seccomp.c
> > > > > @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = {
> > > > >      SCMP_SYS(writev),
> > > > >      SCMP_SYS(umask),
> > > > >      SCMP_SYS(nanosleep),
> > > > > +    SCMP_SYS(clock_nanosleep),
> > > > 
> > > > This patch can be dropped once sleep has been replaced by a condvar.
> > > 
> > > There is another sleep in do_pool_destroy() where we are waiting
> > > for all current threads to exit.
> > > 
> > > do_pool_destroy() {
> > >     g_usleep(10000);
> > > }
> > 
> > That won't be necessary if there's a way to avoid the thread pool :).
> > See my other reply about closing the OFD instead of using signals to
> > cancel blocking fcntl(2).
> 
> Hi Stefan,
> 
> I responded to that email already. man fnctl does not say anything
> about closing fd will unblock the waiter with -EINTR and I had a 
> quick look at kernel code and did not find anything which suggested
> closing fd will unblock current waiters.
> 
> So is this something you know works or you want me to try and see
> if it works?

Thanks for testing it!

Stefan