All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest
@ 2018-05-07  3:32 Yi Min Zhao
  2018-05-07  3:32 ` [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined Yi Min Zhao
  2018-05-07  9:29 ` [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Christian Borntraeger
  0 siblings, 2 replies; 14+ messages in thread
From: Yi Min Zhao @ 2018-05-07  3:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: jtomko, jferlan, berrange, otubo, borntraeger, fiuczy, zyimin

1. Problem Description
======================
If QEMU is built without seccomp support, 'elevatorprivileges' remains compiled.
This option of sandbox is treated as an indication for seccomp blacklist support
in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
startup would fail.

2. Libvirt Log
==============
qemu-system-s390x: -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny: seccomp support is disabled

3. Fixup
========
Wrap the options except 'enable' for qemu_sandbox_opts by CONFIG_SECCOMP.

Yi Min Zhao (1):
  sandbox: avoid to compile options if CONFIG_SECCOMP undefined

 vl.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.15.1 (Apple Git-101)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07  3:32 [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Yi Min Zhao
@ 2018-05-07  3:32 ` Yi Min Zhao
  2018-05-07 10:31   ` Eduardo Otubo
  2018-05-07 18:04   ` Eric Blake
  2018-05-07  9:29 ` [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Christian Borntraeger
  1 sibling, 2 replies; 14+ messages in thread
From: Yi Min Zhao @ 2018-05-07  3:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: jtomko, jferlan, berrange, otubo, borntraeger, fiuczy, zyimin

If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
complied. This would make libvirt set the corresponding capability and
then trigger the guest startup fails. So let's wrap the options with
CONFIG_SECCOMP.

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index fce1fd12d8..cb07b19c02 100644
--- a/vl.c
+++ b/vl.c
@@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
             .name = "enable",
             .type = QEMU_OPT_BOOL,
         },
+#ifdef CONFIG_SECCOMP
         {
             .name = "obsolete",
             .type = QEMU_OPT_STRING,
@@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
             .name = "resourcecontrol",
             .type = QEMU_OPT_STRING,
         },
+#endif
         { /* end of list */ }
     },
 };
-- 
2.15.1 (Apple Git-101)

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest
  2018-05-07  3:32 [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Yi Min Zhao
  2018-05-07  3:32 ` [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined Yi Min Zhao
@ 2018-05-07  9:29 ` Christian Borntraeger
  2018-05-07 10:33   ` Eduardo Otubo
  1 sibling, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2018-05-07  9:29 UTC (permalink / raw)
  To: Yi Min Zhao, qemu-devel
  Cc: jtomko, jferlan, berrange, otubo, fiuczy, libvir-list

On 05/07/2018 05:32 AM, Yi Min Zhao wrote:
> 1. Problem Description
> ======================
> If QEMU is built without seccomp support, 'elevatorprivileges' remains compiled.
> This option of sandbox is treated as an indication for seccomp blacklist support
> in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
> 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
> startup would fail.

Adding libvirt list.

This would still fail with older QEMUs, so the question is if we should also OR instead
change something in libvirt.

> 
> 2. Libvirt Log
> ==============
> qemu-system-s390x: -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> resourcecontrol=deny: seccomp support is disabled
> 
> 3. Fixup
> ========
> Wrap the options except 'enable' for qemu_sandbox_opts by CONFIG_SECCOMP.
> 
> Yi Min Zhao (1):
>   sandbox: avoid to compile options if CONFIG_SECCOMP undefined
> 
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07  3:32 ` [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined Yi Min Zhao
@ 2018-05-07 10:31   ` Eduardo Otubo
  2018-05-07 13:27     ` Yi Min Zhao
  2018-05-07 18:04   ` Eric Blake
  1 sibling, 1 reply; 14+ messages in thread
From: Eduardo Otubo @ 2018-05-07 10:31 UTC (permalink / raw)
  To: Yi Min Zhao; +Cc: qemu-devel, jtomko, jferlan, berrange, borntraeger, fiuczy

On 07/05/2018 - 11:32:14, Yi Min Zhao wrote:
> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
> complied. This would make libvirt set the corresponding capability and
> then trigger the guest startup fails. So let's wrap the options with
> CONFIG_SECCOMP.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
> ---
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index fce1fd12d8..cb07b19c02 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>              .name = "enable",
>              .type = QEMU_OPT_BOOL,
>          },
> +#ifdef CONFIG_SECCOMP
>          {
>              .name = "obsolete",
>              .type = QEMU_OPT_STRING,
> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>              .name = "resourcecontrol",
>              .type = QEMU_OPT_STRING,
>          },
> +#endif
>          { /* end of list */ }
>      },
>  };
> -- 
> 2.15.1 (Apple Git-101)
> 
Acked-by: Eduardo Otubo <otubo@redhat.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest
  2018-05-07  9:29 ` [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Christian Borntraeger
@ 2018-05-07 10:33   ` Eduardo Otubo
  2018-05-07 12:02     ` [Qemu-devel] [libvirt] " Ján Tomko
  0 siblings, 1 reply; 14+ messages in thread
From: Eduardo Otubo @ 2018-05-07 10:33 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Yi Min Zhao, qemu-devel, jtomko, jferlan, berrange, fiuczy, libvir-list

On 07/05/2018 - 11:29:57, Christian Borntraeger wrote:
> On 05/07/2018 05:32 AM, Yi Min Zhao wrote:
> > 1. Problem Description
> > ======================
> > If QEMU is built without seccomp support, 'elevatorprivileges' remains compiled.
> > This option of sandbox is treated as an indication for seccomp blacklist support
> > in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
> > 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
> > startup would fail.
> 
> Adding libvirt list.
> 
> This would still fail with older QEMUs, so the question is if we should also OR instead
> change something in libvirt.

Perhaps I'm missing something here, but libvirt can differentiate between
different versions of QEMU, therefore not calling it with wrong or outdated
arguments.

> 
> > 
> > 2. Libvirt Log
> > ==============
> > qemu-system-s390x: -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> > resourcecontrol=deny: seccomp support is disabled
> > 
> > 3. Fixup
> > ========
> > Wrap the options except 'enable' for qemu_sandbox_opts by CONFIG_SECCOMP.
> > 
> > Yi Min Zhao (1):
> >   sandbox: avoid to compile options if CONFIG_SECCOMP undefined
> > 
> >  vl.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 

-- 
Eduardo Otubo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [libvirt] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest
  2018-05-07 10:33   ` Eduardo Otubo
@ 2018-05-07 12:02     ` Ján Tomko
  2018-05-07 12:12       ` Christian Borntraeger
  0 siblings, 1 reply; 14+ messages in thread
From: Ján Tomko @ 2018-05-07 12:02 UTC (permalink / raw)
  To: Eduardo Otubo
  Cc: Christian Borntraeger, fiuczy, libvir-list, qemu-devel, Yi Min Zhao

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

On Mon, May 07, 2018 at 12:33:20PM +0200, Eduardo Otubo wrote:
>On 07/05/2018 - 11:29:57, Christian Borntraeger wrote:
>> On 05/07/2018 05:32 AM, Yi Min Zhao wrote:
>> > 1. Problem Description
>> > ======================
>> > If QEMU is built without seccomp support, 'elevatorprivileges' remains compiled.
>> > This option of sandbox is treated as an indication for seccomp blacklist support
>> > in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
>> > 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
>> > startup would fail.
>>
>> Adding libvirt list.
>>
>> This would still fail with older QEMUs, so the question is if we should also OR instead
>> change something in libvirt.
>
>Perhaps I'm missing something here, but libvirt can differentiate between
>different versions of QEMU, therefore not calling it with wrong or outdated
>arguments.
>

The code introduced in libvirt commit 31ca6a5 specifically looks for
'elevateprivileges' in 'parameters' of the 'sandbox' option through
query-command-line-options.

Outdated QEMUs should not have this option there.

However, libvirtd does add the option by default not knowing whether it
can fail for other reasons, e.g. SECCOMP not being enabled in the
running kernel. I wonder if that is worth addressing.

Jano

>>
>> >
>> > 2. Libvirt Log
>> > ==============
>> > qemu-system-s390x: -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
>> > resourcecontrol=deny: seccomp support is disabled
>> >
>> > 3. Fixup
>> > ========
>> > Wrap the options except 'enable' for qemu_sandbox_opts by CONFIG_SECCOMP.
>> >
>> > Yi Min Zhao (1):
>> >   sandbox: avoid to compile options if CONFIG_SECCOMP undefined
>> >
>> >  vl.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>>
>
>-- 
>Eduardo Otubo
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [libvirt] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest
  2018-05-07 12:02     ` [Qemu-devel] [libvirt] " Ján Tomko
@ 2018-05-07 12:12       ` Christian Borntraeger
  0 siblings, 0 replies; 14+ messages in thread
From: Christian Borntraeger @ 2018-05-07 12:12 UTC (permalink / raw)
  To: Ján Tomko, Eduardo Otubo
  Cc: fiuczy, libvir-list, qemu-devel, Yi Min Zhao



On 05/07/2018 02:02 PM, Ján Tomko wrote:
> On Mon, May 07, 2018 at 12:33:20PM +0200, Eduardo Otubo wrote:
>> On 07/05/2018 - 11:29:57, Christian Borntraeger wrote:
>>> On 05/07/2018 05:32 AM, Yi Min Zhao wrote:
>>> > 1. Problem Description
>>> > ======================
>>> > If QEMU is built without seccomp support, 'elevatorprivileges' remains compiled.
>>> > This option of sandbox is treated as an indication for seccomp blacklist support
>>> > in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
>>> > 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the guest
>>> > startup would fail.
>>>
>>> Adding libvirt list.
>>>
>>> This would still fail with older QEMUs, so the question is if we should also OR instead
>>> change something in libvirt.
>>
>> Perhaps I'm missing something here, but libvirt can differentiate between
>> different versions of QEMU, therefore not calling it with wrong or outdated
>> arguments.
>>
> 
> The code introduced in libvirt commit 31ca6a5 specifically looks for
> 'elevateprivileges' in 'parameters' of the 'sandbox' option through
> query-command-line-options.
> 
> Outdated QEMUs should not have this option there.
> 
> However, libvirtd does add the option by default not knowing whether it
> can fail for other reasons, e.g. SECCOMP not being enabled in the
> running kernel. I wonder if that is worth addressing.

So you prefer the qemu patch (with cc stable) as the best solution?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07 10:31   ` Eduardo Otubo
@ 2018-05-07 13:27     ` Yi Min Zhao
  0 siblings, 0 replies; 14+ messages in thread
From: Yi Min Zhao @ 2018-05-07 13:27 UTC (permalink / raw)
  To: Eduardo Otubo; +Cc: qemu-devel, jtomko, jferlan, berrange, borntraeger, fiuczy



在 2018/5/7 下午6:31, Eduardo Otubo 写道:
> On 07/05/2018 - 11:32:14, Yi Min Zhao wrote:
>> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
>> complied. This would make libvirt set the corresponding capability and
>> then trigger the guest startup fails. So let's wrap the options with
>> CONFIG_SECCOMP.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>> ---
>>   vl.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index fce1fd12d8..cb07b19c02 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "enable",
>>               .type = QEMU_OPT_BOOL,
>>           },
>> +#ifdef CONFIG_SECCOMP
>>           {
>>               .name = "obsolete",
>>               .type = QEMU_OPT_STRING,
>> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "resourcecontrol",
>>               .type = QEMU_OPT_STRING,
>>           },
>> +#endif
>>           { /* end of list */ }
>>       },
>>   };
>> -- 
>> 2.15.1 (Apple Git-101)
>>
> Acked-by: Eduardo Otubo <otubo@redhat.com>
>
>
Thanks for your review!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07  3:32 ` [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined Yi Min Zhao
  2018-05-07 10:31   ` Eduardo Otubo
@ 2018-05-07 18:04   ` Eric Blake
  2018-05-07 22:18     ` Yi Min Zhao
                       ` (2 more replies)
  1 sibling, 3 replies; 14+ messages in thread
From: Eric Blake @ 2018-05-07 18:04 UTC (permalink / raw)
  To: Yi Min Zhao, qemu-devel; +Cc: otubo, fiuczy, jtomko, borntraeger, jferlan

On 05/06/2018 10:32 PM, Yi Min Zhao wrote:

In the subject line: s/avoid to compile/avoid compiling/

> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains

s/elevator/elevate/

> complied. This would make libvirt set the corresponding capability and

s/complied/compiled/

> then trigger the guest startup fails. So let's wrap the options with
> CONFIG_SECCOMP.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
> ---
>   vl.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index fce1fd12d8..cb07b19c02 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>               .name = "enable",
>               .type = QEMU_OPT_BOOL,
>           },
> +#ifdef CONFIG_SECCOMP
>           {
>               .name = "obsolete",
>               .type = QEMU_OPT_STRING,
> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>               .name = "resourcecontrol",
>               .type = QEMU_OPT_STRING,
>           },
> +#endif

The commit message mentions only 'elevateprivileges' (once the typo is 
fixed), but you are also crippling 'obsolete', 'spawn', and 
'resourcecontrol'.  Perhaps the commit message should call that out 
better?  Or, since libvirt is looking at just 'elevateprivileges', per 
this line in libvirt's qemu_capabilities.c:

src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges", 
QEMU_CAPS_SECCOMP_BLACKLIST },

is it sufficient to just mask out that one option?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07 18:04   ` Eric Blake
@ 2018-05-07 22:18     ` Yi Min Zhao
  2018-05-08 10:37     ` Daniel P. Berrangé
  2018-05-09 14:23     ` Ján Tomko
  2 siblings, 0 replies; 14+ messages in thread
From: Yi Min Zhao @ 2018-05-07 22:18 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: otubo, borntraeger, fiuczy, jtomko, jferlan



在 2018/5/8 上午2:04, Eric Blake 写道:
> On 05/06/2018 10:32 PM, Yi Min Zhao wrote:
>
> In the subject line: s/avoid to compile/avoid compiling/
>
>> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
>
> s/elevator/elevate/
>
>> complied. This would make libvirt set the corresponding capability and
>
> s/complied/compiled/
Thanks for your correction.
>
>> then trigger the guest startup fails. So let's wrap the options with
>> CONFIG_SECCOMP.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>> ---
>>   vl.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index fce1fd12d8..cb07b19c02 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "enable",
>>               .type = QEMU_OPT_BOOL,
>>           },
>> +#ifdef CONFIG_SECCOMP
>>           {
>>               .name = "obsolete",
>>               .type = QEMU_OPT_STRING,
>> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "resourcecontrol",
>>               .type = QEMU_OPT_STRING,
>>           },
>> +#endif
>
> The commit message mentions only 'elevateprivileges' (once the typo is 
> fixed), but you are also crippling 'obsolete', 'spawn', and 
> 'resourcecontrol'.  Perhaps the commit message should call that out 
> better?  Or, since libvirt is looking at just 'elevateprivileges', per 
> this line in libvirt's qemu_capabilities.c:
>
> src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges", 
> QEMU_CAPS_SECCOMP_BLACKLIST },
>
> is it sufficient to just mask out that one option?
>
The problem is caused by 'elevateprivileges' ostensibly from Libvirt 
point of view. But in Qemu, CONFIG_SECCOMP controls other options except 
'enable' in parse_sandbox(). So if SECCOMP is not configured, we don't 
need other options instead of only elevateprivileges.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07 18:04   ` Eric Blake
  2018-05-07 22:18     ` Yi Min Zhao
@ 2018-05-08 10:37     ` Daniel P. Berrangé
  2018-05-09  4:40       ` Yi Min Zhao
  2018-05-09 14:23     ` Ján Tomko
  2 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrangé @ 2018-05-08 10:37 UTC (permalink / raw)
  To: Eric Blake
  Cc: Yi Min Zhao, qemu-devel, otubo, borntraeger, fiuczy, jtomko, jferlan

On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote:
> On 05/06/2018 10:32 PM, Yi Min Zhao wrote:
> 
> In the subject line: s/avoid to compile/avoid compiling/
> 
> > If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
> 
> s/elevator/elevate/
> 
> > complied. This would make libvirt set the corresponding capability and
> 
> s/complied/compiled/
> 
> > then trigger the guest startup fails. So let's wrap the options with
> > CONFIG_SECCOMP.
> > 
> > Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
> > ---
> >   vl.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index fce1fd12d8..cb07b19c02 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
> >               .name = "enable",
> >               .type = QEMU_OPT_BOOL,
> >           },
> > +#ifdef CONFIG_SECCOMP
> >           {
> >               .name = "obsolete",
> >               .type = QEMU_OPT_STRING,
> > @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
> >               .name = "resourcecontrol",
> >               .type = QEMU_OPT_STRING,
> >           },
> > +#endif
> 
> The commit message mentions only 'elevateprivileges' (once the typo is
> fixed), but you are also crippling 'obsolete', 'spawn', and
> 'resourcecontrol'.  Perhaps the commit message should call that out better?
> Or, since libvirt is looking at just 'elevateprivileges', per this line in
> libvirt's qemu_capabilities.c:
> 
> src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges",
> QEMU_CAPS_SECCOMP_BLACKLIST },
> 
> is it sufficient to just mask out that one option?

If seccomp is disabled, we should really disable the entire -sandbox
argument, not merly the options to it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-08 10:37     ` Daniel P. Berrangé
@ 2018-05-09  4:40       ` Yi Min Zhao
  2018-05-09 12:48         ` Eric Blake
  0 siblings, 1 reply; 14+ messages in thread
From: Yi Min Zhao @ 2018-05-09  4:40 UTC (permalink / raw)
  To: qemu-devel



在 2018/5/8 下午6:37, Daniel P. Berrangé 写道:
> On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote:
>> On 05/06/2018 10:32 PM, Yi Min Zhao wrote:
>>
>> In the subject line: s/avoid to compile/avoid compiling/
>>
>>> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
>> s/elevator/elevate/
>>
>>> complied. This would make libvirt set the corresponding capability and
>> s/complied/compiled/
>>
>>> then trigger the guest startup fails. So let's wrap the options with
>>> CONFIG_SECCOMP.
>>>
>>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>>> ---
>>>    vl.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index fce1fd12d8..cb07b19c02 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>>                .name = "enable",
>>>                .type = QEMU_OPT_BOOL,
>>>            },
>>> +#ifdef CONFIG_SECCOMP
>>>            {
>>>                .name = "obsolete",
>>>                .type = QEMU_OPT_STRING,
>>> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>>                .name = "resourcecontrol",
>>>                .type = QEMU_OPT_STRING,
>>>            },
>>> +#endif
>> The commit message mentions only 'elevateprivileges' (once the typo is
>> fixed), but you are also crippling 'obsolete', 'spawn', and
>> 'resourcecontrol'.  Perhaps the commit message should call that out better?
>> Or, since libvirt is looking at just 'elevateprivileges', per this line in
>> libvirt's qemu_capabilities.c:
>>
>> src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges",
>> QEMU_CAPS_SECCOMP_BLACKLIST },
>>
>> is it sufficient to just mask out that one option?
> If seccomp is disabled, we should really disable the entire -sandbox
> argument, not merly the options to it.
I think it would bring a lot of changes if disable the entire -sandbox 
argument.
Looking from current code, sandbox is a default qemu option group, and 
sandbox.enable is false by default unless you obviously define it with true.
So, this patch is an easier way to fixup.
>
> Regards,
> Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-09  4:40       ` Yi Min Zhao
@ 2018-05-09 12:48         ` Eric Blake
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2018-05-09 12:48 UTC (permalink / raw)
  To: Yi Min Zhao, qemu-devel

On 05/08/2018 11:40 PM, Yi Min Zhao wrote:

>> If seccomp is disabled, we should really disable the entire -sandbox
>> argument, not merly the options to it.
> I think it would bring a lot of changes if disable the entire -sandbox 
> argument.
> Looking from current code, sandbox is a default qemu option group, and 
> sandbox.enable is false by default unless you obviously define it with 
> true.
> So, this patch is an easier way to fixup.

If the only thing you can do with -sandbox is turn it off (which is its 
default state), it's better to not advertise it at all in the first 
place.  I agree with Daniel that it's better to cripple -sandbox from 
even being usable as a command-line argument if it isn't going to work, 
as that's easier to introspect.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined
  2018-05-07 18:04   ` Eric Blake
  2018-05-07 22:18     ` Yi Min Zhao
  2018-05-08 10:37     ` Daniel P. Berrangé
@ 2018-05-09 14:23     ` Ján Tomko
  2 siblings, 0 replies; 14+ messages in thread
From: Ján Tomko @ 2018-05-09 14:23 UTC (permalink / raw)
  To: Eric Blake; +Cc: Yi Min Zhao, qemu-devel, otubo, fiuczy, borntraeger, jferlan

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

On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote:
>On 05/06/2018 10:32 PM, Yi Min Zhao wrote:
>
>In the subject line: s/avoid to compile/avoid compiling/
>
>> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
>
>s/elevator/elevate/
>
>> complied. This would make libvirt set the corresponding capability and
>
>s/complied/compiled/
>
>> then trigger the guest startup fails. So let's wrap the options with
>> CONFIG_SECCOMP.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>> ---
>>   vl.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index fce1fd12d8..cb07b19c02 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "enable",
>>               .type = QEMU_OPT_BOOL,
>>           },
>> +#ifdef CONFIG_SECCOMP
>>           {
>>               .name = "obsolete",
>>               .type = QEMU_OPT_STRING,
>> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
>>               .name = "resourcecontrol",
>>               .type = QEMU_OPT_STRING,
>>           },
>> +#endif
>
>The commit message mentions only 'elevateprivileges' (once the typo is
>fixed), but you are also crippling 'obsolete', 'spawn', and
>'resourcecontrol'.  Perhaps the commit message should call that out
>better?  Or, since libvirt is looking at just 'elevateprivileges', per
>this line in libvirt's qemu_capabilities.c:
>
>src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges",
>QEMU_CAPS_SECCOMP_BLACKLIST },
>
>is it sufficient to just mask out that one option?

That would be inconsistent. I picked one option randomly, because they
were added at the same time, but compiling out just one out of four
is just odd. And with leaving them in even though the functionality is
compiled out, libvirt has no way to tell upfront whether it's usable.

By that logic, removing the -sandbox option without CONFIG_SECCOMP makes
sense, but libvirt already assumes this option is present on all supported
QEMUs (>= 1.5.0), so please cc: me on that change if you decide to
remove it as well.

Jano

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-05-09 14:23 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-07  3:32 [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Yi Min Zhao
2018-05-07  3:32 ` [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined Yi Min Zhao
2018-05-07 10:31   ` Eduardo Otubo
2018-05-07 13:27     ` Yi Min Zhao
2018-05-07 18:04   ` Eric Blake
2018-05-07 22:18     ` Yi Min Zhao
2018-05-08 10:37     ` Daniel P. Berrangé
2018-05-09  4:40       ` Yi Min Zhao
2018-05-09 12:48         ` Eric Blake
2018-05-09 14:23     ` Ján Tomko
2018-05-07  9:29 ` [Qemu-devel] [PATCH 0/1] Bug: Sandbox: libvirt breakdowns qemu guest Christian Borntraeger
2018-05-07 10:33   ` Eduardo Otubo
2018-05-07 12:02     ` [Qemu-devel] [libvirt] " Ján Tomko
2018-05-07 12:12       ` Christian Borntraeger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.