* [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
@ 2019-02-27 1:21 David Gibson
2019-02-27 9:51 ` Marc-André Lureau
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: David Gibson @ 2019-02-27 1:21 UTC (permalink / raw)
To: otubo, pbonzini; +Cc: qemu-devel, qemu-ppc, David Gibson
At present, when seccomp support is compiled out with --disable-seccomp
we fail with an error if the user puts -sandbox on the command line.
That kind of makes sense, but it's a bit strange that we reject a request
to disable sandboxing with "-sandbox off" saying we don't support
sandboxing.
This puts in a small sandbox to (correctly) silently ignore -sandbox off
when we don't have sandboxing support compiled in. This makes life easier
for testcases, since they can safely specify "-sandbox off" without having
to care if the qemu they're using is compiled with sandbox support or not.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
vl.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/vl.c b/vl.c
index 502857a176..9d5f1b7ebb 100644
--- a/vl.c
+++ b/vl.c
@@ -3857,9 +3857,11 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
#else
- error_report("-sandbox support is not enabled "
- "in this QEMU binary");
- exit(1);
+ if (!g_str_equal(optarg, "off")) {
+ error_report("-sandbox support is not enabled "
+ "in this QEMU binary");
+ exit(1);
+ }
#endif
break;
case QEMU_OPTION_add_fd:
--
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 1:21 [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp David Gibson
@ 2019-02-27 9:51 ` Marc-André Lureau
2019-02-27 10:39 ` Stefano Garzarella
2019-02-27 11:01 ` Daniel P. Berrangé
2 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-02-27 9:51 UTC (permalink / raw)
To: David Gibson; +Cc: Eduardo Otubo, Paolo Bonzini, open list:sPAPR pseries, QEMU
Hi
On Wed, Feb 27, 2019 at 2:22 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> At present, when seccomp support is compiled out with --disable-seccomp
> we fail with an error if the user puts -sandbox on the command line.
>
> That kind of makes sense, but it's a bit strange that we reject a request
> to disable sandboxing with "-sandbox off" saying we don't support
> sandboxing.
>
> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> when we don't have sandboxing support compiled in. This makes life easier
> for testcases, since they can safely specify "-sandbox off" without having
> to care if the qemu they're using is compiled with sandbox support or not.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> vl.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 502857a176..9d5f1b7ebb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3857,9 +3857,11 @@ int main(int argc, char **argv, char **envp)
> exit(1);
> }
> #else
> - error_report("-sandbox support is not enabled "
> - "in this QEMU binary");
> - exit(1);
> + if (!g_str_equal(optarg, "off")) {
> + error_report("-sandbox support is not enabled "
> + "in this QEMU binary");
> + exit(1);
> + }
> #endif
> break;
> case QEMU_OPTION_add_fd:
> --
> 2.20.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 1:21 [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp David Gibson
2019-02-27 9:51 ` Marc-André Lureau
@ 2019-02-27 10:39 ` Stefano Garzarella
2019-02-27 11:01 ` Daniel P. Berrangé
2 siblings, 0 replies; 8+ messages in thread
From: Stefano Garzarella @ 2019-02-27 10:39 UTC (permalink / raw)
To: David Gibson; +Cc: otubo, pbonzini, qemu-ppc, qemu-devel
On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
> At present, when seccomp support is compiled out with --disable-seccomp
> we fail with an error if the user puts -sandbox on the command line.
>
> That kind of makes sense, but it's a bit strange that we reject a request
> to disable sandboxing with "-sandbox off" saying we don't support
> sandboxing.
>
> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> when we don't have sandboxing support compiled in. This makes life easier
> for testcases, since they can safely specify "-sandbox off" without having
> to care if the qemu they're using is compiled with sandbox support or not.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> vl.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 1:21 [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp David Gibson
2019-02-27 9:51 ` Marc-André Lureau
2019-02-27 10:39 ` Stefano Garzarella
@ 2019-02-27 11:01 ` Daniel P. Berrangé
2019-02-27 13:59 ` Eric Blake
2 siblings, 1 reply; 8+ messages in thread
From: Daniel P. Berrangé @ 2019-02-27 11:01 UTC (permalink / raw)
To: David Gibson; +Cc: otubo, pbonzini, qemu-ppc, qemu-devel
On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
> At present, when seccomp support is compiled out with --disable-seccomp
> we fail with an error if the user puts -sandbox on the command line.
>
> That kind of makes sense, but it's a bit strange that we reject a request
> to disable sandboxing with "-sandbox off" saying we don't support
> sandboxing.
>
> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> when we don't have sandboxing support compiled in. This makes life easier
> for testcases, since they can safely specify "-sandbox off" without having
> to care if the qemu they're using is compiled with sandbox support or not.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> vl.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 502857a176..9d5f1b7ebb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3857,9 +3857,11 @@ int main(int argc, char **argv, char **envp)
> exit(1);
> }
> #else
> - error_report("-sandbox support is not enabled "
> - "in this QEMU binary");
> - exit(1);
> + if (!g_str_equal(optarg, "off")) {
> + error_report("-sandbox support is not enabled "
> + "in this QEMU binary");
> + exit(1);
> + }
'-sandbox off' is just syntax sugar for '-sandbox enable=off', with
the default arg name handled by QemuOpts.
If we want to keep ability to run "-sandbox off" we should do it via
the QEMU opts code we already have in the first part of the conditional,
so that "-sandbox enable=off" also works as normal.
Essentially we need to push the #ifdef CONFIG_SECCOMP down into the
parse_sandbox method, so that it parses the "enable" option normally
but rejects any value except disabled..
The QemuOptsList should also be conditional to only register the
"enable" arg and not the other bits.
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] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 11:01 ` Daniel P. Berrangé
@ 2019-02-27 13:59 ` Eric Blake
2019-02-27 14:08 ` Daniel P. Berrangé
0 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2019-02-27 13:59 UTC (permalink / raw)
To: Daniel P. Berrangé, David Gibson
Cc: otubo, pbonzini, qemu-ppc, qemu-devel
On 2/27/19 5:01 AM, Daniel P. Berrangé wrote:
> On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
>> At present, when seccomp support is compiled out with --disable-seccomp
>> we fail with an error if the user puts -sandbox on the command line.
>>
>> That kind of makes sense, but it's a bit strange that we reject a request
>> to disable sandboxing with "-sandbox off" saying we don't support
>> sandboxing.
>>
>> This puts in a small sandbox to (correctly) silently ignore -sandbox off
>> when we don't have sandboxing support compiled in. This makes life easier
>> for testcases, since they can safely specify "-sandbox off" without having
>> to care if the qemu they're using is compiled with sandbox support or not.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> '-sandbox off' is just syntax sugar for '-sandbox enable=off', with
> the default arg name handled by QemuOpts.
Except that libvirt probes, via query-command-line-options, whether the
'-sandbox' option supports the 'enable' key. You risk breaking
introspection (where libvirt knows NOT to use enable=on|off) if -sandbox
enable=off is advertised even when the feature is not compiled in.
>
> If we want to keep ability to run "-sandbox off" we should do it via
> the QEMU opts code we already have in the first part of the conditional,
> so that "-sandbox enable=off" also works as normal.
>
> Essentially we need to push the #ifdef CONFIG_SECCOMP down into the
> parse_sandbox method, so that it parses the "enable" option normally
> but rejects any value except disabled..
>
> The QemuOptsList should also be conditional to only register the
> "enable" arg and not the other bits.
Whatever we do here had better be careful that the introspection used by
libvirt doesn't break, when libvirt is trying to learn when seccomp was
not compiled in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 13:59 ` Eric Blake
@ 2019-02-27 14:08 ` Daniel P. Berrangé
2019-02-27 19:21 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Daniel P. Berrangé @ 2019-02-27 14:08 UTC (permalink / raw)
To: Eric Blake; +Cc: David Gibson, otubo, pbonzini, qemu-ppc, qemu-devel
On Wed, Feb 27, 2019 at 07:59:11AM -0600, Eric Blake wrote:
> On 2/27/19 5:01 AM, Daniel P. Berrangé wrote:
> > On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
> >> At present, when seccomp support is compiled out with --disable-seccomp
> >> we fail with an error if the user puts -sandbox on the command line.
> >>
> >> That kind of makes sense, but it's a bit strange that we reject a request
> >> to disable sandboxing with "-sandbox off" saying we don't support
> >> sandboxing.
> >>
> >> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> >> when we don't have sandboxing support compiled in. This makes life easier
> >> for testcases, since they can safely specify "-sandbox off" without having
> >> to care if the qemu they're using is compiled with sandbox support or not.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> > '-sandbox off' is just syntax sugar for '-sandbox enable=off', with
> > the default arg name handled by QemuOpts.
>
> Except that libvirt probes, via query-command-line-options, whether the
> '-sandbox' option supports the 'enable' key. You risk breaking
> introspection (where libvirt knows NOT to use enable=on|off) if -sandbox
> enable=off is advertised even when the feature is not compiled in.
It is even more complex than that. Libvirt looks for "elevateprivileges"
option to decide to enable the sandbox. It only looks for "enable" when
libvirt is configured to disable the sandbox, at which point is sets
"-sandbox off". So I don't think my suggestion should break it.
I do hate the idea of QEMU tailoring its CLI handling to suit the
current specific impl of one client app though.
If anything I'd suggest we should completely disable any parsing
of -sandbox when seccomp is disabled, rather than leaving getopt
to parse -sandbox and then raise an error.
>
> >
> > If we want to keep ability to run "-sandbox off" we should do it via
> > the QEMU opts code we already have in the first part of the conditional,
> > so that "-sandbox enable=off" also works as normal.
> >
> > Essentially we need to push the #ifdef CONFIG_SECCOMP down into the
> > parse_sandbox method, so that it parses the "enable" option normally
> > but rejects any value except disabled..
> >
> > The QemuOptsList should also be conditional to only register the
> > "enable" arg and not the other bits.
>
> Whatever we do here had better be careful that the introspection used by
> libvirt doesn't break, when libvirt is trying to learn when seccomp was
> not compiled in.
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] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 14:08 ` Daniel P. Berrangé
@ 2019-02-27 19:21 ` Markus Armbruster
2019-02-28 10:22 ` Daniel P. Berrangé
0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2019-02-27 19:21 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eric Blake, otubo, pbonzini, qemu-ppc, qemu-devel, David Gibson
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Wed, Feb 27, 2019 at 07:59:11AM -0600, Eric Blake wrote:
>> On 2/27/19 5:01 AM, Daniel P. Berrangé wrote:
>> > On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
>> >> At present, when seccomp support is compiled out with --disable-seccomp
>> >> we fail with an error if the user puts -sandbox on the command line.
>> >>
>> >> That kind of makes sense, but it's a bit strange that we reject a request
>> >> to disable sandboxing with "-sandbox off" saying we don't support
>> >> sandboxing.
>> >>
>> >> This puts in a small sandbox to (correctly) silently ignore -sandbox off
>> >> when we don't have sandboxing support compiled in. This makes life easier
>> >> for testcases, since they can safely specify "-sandbox off" without having
>> >> to care if the qemu they're using is compiled with sandbox support or not.
I can't see such test cases. Can you give specific examples?
>> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> > '-sandbox off' is just syntax sugar for '-sandbox enable=off', with
>> > the default arg name handled by QemuOpts.
>>
>> Except that libvirt probes, via query-command-line-options, whether the
>> '-sandbox' option supports the 'enable' key. You risk breaking
>> introspection (where libvirt knows NOT to use enable=on|off) if -sandbox
>> enable=off is advertised even when the feature is not compiled in.
>
> It is even more complex than that. Libvirt looks for "elevateprivileges"
> option to decide to enable the sandbox. It only looks for "enable" when
> libvirt is configured to disable the sandbox, at which point is sets
> "-sandbox off". So I don't think my suggestion should break it.
>
> I do hate the idea of QEMU tailoring its CLI handling to suit the
> current specific impl of one client app though.
>
> If anything I'd suggest we should completely disable any parsing
> of -sandbox when seccomp is disabled, rather than leaving getopt
> to parse -sandbox and then raise an error.
I'm confused. Are you proposing to silently ignore -sandbox OPTARG
regardless of OPTARG when CONFIG_SECCOMP is off? If yes, I object. If
a user asks for a sandbox, and we can't give him one, we should at least
tell him as much.
>> > If we want to keep ability to run "-sandbox off" we should do it via
>> > the QEMU opts code we already have in the first part of the conditional,
>> > so that "-sandbox enable=off" also works as normal.
>> >
>> > Essentially we need to push the #ifdef CONFIG_SECCOMP down into the
>> > parse_sandbox method, so that it parses the "enable" option normally
>> > but rejects any value except disabled..
>> >
>> > The QemuOptsList should also be conditional to only register the
>> > "enable" arg and not the other bits.
>>
>> Whatever we do here had better be careful that the introspection used by
>> libvirt doesn't break, when libvirt is trying to learn when seccomp was
>> not compiled in.
Currently, we have both
#ifdef CONFIG_FOO
case QEMU_OPTION_FOO:
...
break;
#endif
and
case QEMU_OPTION_FOO:
#ifdef CONFIG_FOO
...
break;
#else
error_report("FOO support is disabled");
exit(1);
#endif
The patch adds a third variant. We need fewer variants, not more.
For what it's worth, conditional QMP commands do not exist when
disabled, like the first variant above. In particular, introspection
doesn't have them. Nicely obvious.
QAPIfying the command line (yes, yes, overdue) should make it more like
QMP.
Wanting nicer errors than "unknown option / command" is legitimate.
Wanting to accept "switch FOO off" even though FOO is disabled is also
legitimate.
Patches that provide such UI niceties while keeping introspection nicely
obvious would be acceptable to me, provided they're not too complex.
But they should be general, not a one-off.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
2019-02-27 19:21 ` Markus Armbruster
@ 2019-02-28 10:22 ` Daniel P. Berrangé
0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2019-02-28 10:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eric Blake, otubo, pbonzini, qemu-ppc, qemu-devel, David Gibson
On Wed, Feb 27, 2019 at 08:21:40PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Wed, Feb 27, 2019 at 07:59:11AM -0600, Eric Blake wrote:
> >> On 2/27/19 5:01 AM, Daniel P. Berrangé wrote:
> >> > On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
> >> >> At present, when seccomp support is compiled out with --disable-seccomp
> >> >> we fail with an error if the user puts -sandbox on the command line.
> >> >>
> >> >> That kind of makes sense, but it's a bit strange that we reject a request
> >> >> to disable sandboxing with "-sandbox off" saying we don't support
> >> >> sandboxing.
> >> >>
> >> >> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> >> >> when we don't have sandboxing support compiled in. This makes life easier
> >> >> for testcases, since they can safely specify "-sandbox off" without having
> >> >> to care if the qemu they're using is compiled with sandbox support or not.
>
> I can't see such test cases. Can you give specific examples?
>
> >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>
> >> > '-sandbox off' is just syntax sugar for '-sandbox enable=off', with
> >> > the default arg name handled by QemuOpts.
> >>
> >> Except that libvirt probes, via query-command-line-options, whether the
> >> '-sandbox' option supports the 'enable' key. You risk breaking
> >> introspection (where libvirt knows NOT to use enable=on|off) if -sandbox
> >> enable=off is advertised even when the feature is not compiled in.
> >
> > It is even more complex than that. Libvirt looks for "elevateprivileges"
> > option to decide to enable the sandbox. It only looks for "enable" when
> > libvirt is configured to disable the sandbox, at which point is sets
> > "-sandbox off". So I don't think my suggestion should break it.
> >
> > I do hate the idea of QEMU tailoring its CLI handling to suit the
> > current specific impl of one client app though.
> >
> > If anything I'd suggest we should completely disable any parsing
> > of -sandbox when seccomp is disabled, rather than leaving getopt
> > to parse -sandbox and then raise an error.
>
> I'm confused. Are you proposing to silently ignore -sandbox OPTARG
> regardless of OPTARG when CONFIG_SECCOMP is off? If yes, I object. If
> a user asks for a sandbox, and we can't give him one, we should at least
> tell him as much.
No, i mean remove "case QEMU_OPTION_sandbox:" from the code, which
will cause us to report '-sandbox' as an unsupported argument.
> Currently, we have both
>
> #ifdef CONFIG_FOO
> case QEMU_OPTION_FOO:
> ...
> break;
> #endif
This is what I was suggesting for -sandbox above.
>
> and
>
>
> case QEMU_OPTION_FOO:
> #ifdef CONFIG_FOO
> ...
> break;
> #else
> error_report("FOO support is disabled");
> exit(1);
> #endif
This is what -sandbox does currently.
>
> The patch adds a third variant. We need fewer variants, not more.
>
> For what it's worth, conditional QMP commands do not exist when
> disabled, like the first variant above. In particular, introspection
> doesn't have them. Nicely obvious.
>
> QAPIfying the command line (yes, yes, overdue) should make it more like
> QMP.
>
> Wanting nicer errors than "unknown option / command" is legitimate.
>
> Wanting to accept "switch FOO off" even though FOO is disabled is also
> legitimate.
>
> Patches that provide such UI niceties while keeping introspection nicely
> obvious would be acceptable to me, provided they're not too complex.
> But they should be general, not a one-off.
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] 8+ messages in thread
end of thread, other threads:[~2019-02-28 10:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 1:21 [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp David Gibson
2019-02-27 9:51 ` Marc-André Lureau
2019-02-27 10:39 ` Stefano Garzarella
2019-02-27 11:01 ` Daniel P. Berrangé
2019-02-27 13:59 ` Eric Blake
2019-02-27 14:08 ` Daniel P. Berrangé
2019-02-27 19:21 ` Markus Armbruster
2019-02-28 10:22 ` Daniel P. Berrangé
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.