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

[Jason Wang]

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
>