Re: [Qemu-devel] [PATCHv2 4/4] Warning messages on net devices hotplug

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 10/23/2012 01:55 AM, Eduardo Otubo wrote:
> With the inclusion of the new "double whitelist" seccomp filter, Qemu
> won't be able to execve() in runtime, thus, no hotplug net devices
> allowed.
>
> v2: * Error messages moved to the backend function, net_init_tap(), recommended
>        by Paolo Bonzini
>      * Documentation added to QMP and HMP commands, and also to the Qemu options.
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>   hmp-commands.hx | 4 ++--
>   net.c           | 1 +
>   net/tap.c       | 5 +++++
>   qemu-options.hx | 3 ++-
>   qmp-commands.hx | 3 ++-
>   5 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e0b537d..3e28501 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1068,7 +1068,7 @@ ETEXI
>           .name       = "host_net_add",
>           .args_type  = "device:s,opts:s?",
>           .params     = "tap|user|socket|vde|dump [options]",
> -        .help       = "add host VLAN client",
> +        .help       = "add host VLAN client, feature disabled when -sandbox is in use",

Maybe the "feature disabled.." part should be in parenthesis?  There's 
also another section in hmp-commands.hx a few lines below this code 
where you can add the same note.

>           .mhandler.cmd = net_host_device_add,
>       },
>
> @@ -1096,7 +1096,7 @@ ETEXI
>           .name       = "netdev_add",
>           .args_type  = "netdev:O",
>           .params     = "[user|tap|socket],id=str[,prop=value][,...]",
> -        .help       = "add host network device",
> +        .help       = "add host network device, feature disabled when -sandbox is in use",

Same comments as above.

>           .mhandler.cmd = hmp_netdev_add,
>       },
>
> diff --git a/net.c b/net.c
> index ae4bc0d..02188f0 100644
> --- a/net.c
> +++ b/net.c
> @@ -765,6 +765,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>       qemu_opt_set(opts, "type", device);
>
>       net_client_init(opts, 0, &local_err);
> +

You can get rid of this change.

>       if (error_is_set(&local_err)) {
>           qerror_report_err(local_err);
>           error_free(local_err);
> diff --git a/net/tap.c b/net/tap.c
> index df89caa..dd8c79b 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -590,6 +590,11 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>   int net_init_tap(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer)
>   {
> +#ifdef CONFIG_SECCOMP
> +    error_report("Cannot hotplug TAP device when -sandbox is in effect");
> +    return -1;
> +#endif
> +

It seems like it would make more sense to put this code after the local 
variables are defined.  But more importantly, does this prevent adding a 
tap device on the command line?  If so, that's not good.  Also I don't 
think you are preventing a "netdev_add bridge" any more in v2, which 
also calls execve().

>       const NetdevTapOptions *tap;
>
>       int fd, vnet_hdr = 0;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7d97f96..02afba3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2767,7 +2767,8 @@ STEXI
>   @item -sandbox
>   @findex -sandbox
>   Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will
> -disable it.  The default is 'off'.
> +disable it.  The default is 'on'. Note that when using the '-sandbox on' option the hot plug
> +of new devices will be disabled.

Only network devices are prevented, right?

Also, as I mentioned before, can you limit this to the subset of options 
that cause execve() to be issued?  For example, can we allow libvirt to 
pass an fd for hotplugging a network device (e.g. netdev_add tap,fd=23)? 
  I don't know for sure but I'm guessing libvirt does that.

Also I think it would be worth-while to update the qemu-kvm --help 
output with a similar note too.

>   ETEXI
>
>   DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2f8477e..cccb8f1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -757,7 +757,8 @@ Example:
>
>   Note: The supported device options are the same ones supported by the '-net'
>         command-line argument, which are listed in the '-help' output or QEMU's
> -      manual
> +      manual. Also note that the hot plug is disabled when -sandbox is in
> +      effect

Not all hotplug abilities are disabled.  Just network devices.  This is 
missing a period too.

>
>   EQMP
>

-- 
Regards,
Corey Bryant