[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing bot
From: |
Jason Wang |
Subject: |
Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap |
Date: |
Tue, 04 Jun 2013 13:25:22 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 |
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 <address@hidden>
>> Cc: Stefan Hajnoczi <address@hidden>
>> Cc: Laszlo Ersek <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Jason Wang <address@hidden>
>> ---
>> 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
>