From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjSHf-0006L2-EH for qemu-devel@nongnu.org; Mon, 03 Jun 2013 06:46:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UjSHW-0003If-5W for qemu-devel@nongnu.org; Mon, 03 Jun 2013 06:46:39 -0400 Message-ID: <51AC7492.9030902@redhat.com> Date: Mon, 03 Jun 2013 12:48:50 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1370250244-30058-1-git-send-email-jasowang@redhat.com> In-Reply-To: <1370250244-30058-1-git-send-email-jasowang@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org, Stefan Hajnoczi , stefanha@redhat.com, Paolo Bonzini 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 > Cc: Stefan Hajnoczi > Cc: Laszlo Ersek > Cc: qemu-stable@nongnu.org > Signed-off-by: Jason Wang > --- > 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