Patchwork [PATCHv2,4/4] Warning messages on net devices hotplug

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(-)

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
>

On 10/23/2012 09:59 AM, Corey Bryant wrote:

> 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.

Correct, libvirt prefers passing network devices pre-opened via fds,
rather than having qemu exec scripts.

>> +      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.

And not all network hotplug, just hotplug that requires use of exec
(again, fd passing bypasses the need for exec).