From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ujjko-0007uw-1c for qemu-devel@nongnu.org; Tue, 04 Jun 2013 01:25:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ujjkj-0000ua-5C for qemu-devel@nongnu.org; Tue, 04 Jun 2013 01:25:53 -0400 Message-ID: <51AD7A42.1020807@redhat.com> Date: Tue, 04 Jun 2013 13:25:22 +0800 From: Jason Wang MIME-Version: 1.0 References: <1370250244-30058-1-git-send-email-jasowang@redhat.com> <51AC7492.9030902@redhat.com> In-Reply-To: <51AC7492.9030902@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: Laszlo Ersek Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org, Stefan Hajnoczi , stefanha@redhat.com, Paolo Bonzini On 06/03/2013 06:48 PM, Laszlo Ersek wrote: > 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. Ture > > > 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. Sure will send v2. Thanks > Thanks > Laszlo >