Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 06/03/13 11:04, Jason Wang wrote:
> This is because vhostfdname were passed as NULL to net_init_tap_one() when
> vhostfd were not specified, but net_init_tap_one() will still pass it to
> monitor_handle_fd_param() when tap->has_vhostfds is true. Since file descriptor
> (fd, vhostfd) and file descriptor set (fds, vhostfds) were not compatible, so
> this patch forbids passing them to tap in the same time.
> 
> This solve the segfault when passing the command line like:
> ./qemu-system-x86_64 -netdev tap,fd=2,vhost=on,vhostfds=baz,id=xyz
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <shajnocz@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/tap.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index e0b7a2a..477505f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -698,9 +698,10 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>      if (tap->has_fd) {
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>              tap->has_vnet_hdr || tap->has_helper || tap->has_queues ||
> -            tap->has_fds) {
> +            tap->has_fds || tap->has_vhostfds) {
>              error_report("ifname=, script=, downscript=, vnet_hdr=, "
> -                         "helper=, queues=, and fds= are invalid with fd=");
> +                         "helper=, queues=, fds=, and vhostfds= "
> +                         "are invalid with fd=");
>              return -1;
>          }
>  

This seems OK, since

has_fd                   precludes    has_fds
- optional: has_vhostfd               - optional: has_vhostfds

> @@ -725,9 +726,10 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>  
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>              tap->has_vnet_hdr || tap->has_helper || tap->has_queues ||
> -            tap->has_fd) {
> +            tap->has_fd || tap->has_vhostfd) {
>              error_report("ifname=, script=, downscript=, vnet_hdr=, "
> -                         "helper=, queues=, and fd= are invalid with fds=");
> +                         "helper=, queues=, fd=, and vhostfd= "
> +                         "are invalid with fds=");
>              return -1;
>          }
>  
> 

I think this us OK too (implementing the above exclusion from the other
side), but in the second hunk "tap->has_fd" can never be true actually.
I think relying on that in both the condition and the error message here
would simplify them.


Finally, I think the "has_helper", and the outermost "else" branch
should both be updated as well, for consistency with hunk #1 here. These
branches use "vhostfdname" too. In these branches,
- both "has_fd" and "has_fds" are false (should simplify check & errmsg),
- "has_vhostfds" should be rejected.

Thanks
Laszlo