* [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap
@ 2013-06-03 9:04 Jason Wang
2013-06-03 10:48 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Jason Wang @ 2013-06-03 9:04 UTC (permalink / raw)
To: aliguori, stefanha, qemu-devel
Cc: Paolo Bonzini, Jason Wang, Stefan Hajnoczi, qemu-stable, Laszlo Ersek
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;
}
@@ -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;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap
2013-06-03 9:04 [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap Jason Wang
@ 2013-06-03 10:48 ` Laszlo Ersek
2013-06-04 5:25 ` Jason Wang
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2013-06-03 10:48 UTC (permalink / raw)
To: Jason Wang
Cc: aliguori, qemu-devel, qemu-stable, Stefan Hajnoczi, stefanha,
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 <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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap
2013-06-03 10:48 ` Laszlo Ersek
@ 2013-06-04 5:25 ` Jason Wang
0 siblings, 0 replies; 3+ messages in thread
From: Jason Wang @ 2013-06-04 5:25 UTC (permalink / raw)
To: Laszlo Ersek
Cc: aliguori, qemu-devel, qemu-stable, Stefan Hajnoczi, stefanha,
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 <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.
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-04 5:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-03 9:04 [Qemu-devel] [PATCH] net: tap: fix NULL dereference when passing both fd and vhostfds to tap Jason Wang
2013-06-03 10:48 ` Laszlo Ersek
2013-06-04 5:25 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.