[Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Andrew Baumann]

Minor revisions to the second patch in the series.

Cheers,
Andrew

Andrew Baumann (2):
  tap-win32: skip unexpected nodes during registry enumeration
  tap-win32: disable broken async write path

 net/tap-win32.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 11 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH v2 1/2] tap-win32: skip unexpected nodes during registry enumeration

[Andrew Baumann]

In order to find a named tap device, get_device_guid() enumerates children of
HKLM\SYSTEM\CCS\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318}
(aka NETWORK_CONNECTIONS_KEY). For each child, it then looks for a
"Connection" subkey, but if this key doesn't exist, it aborts the
entire search. This was observed to fail on at least one Windows 10
machine, where there is an additional child of NETWORK_CONNECTIONS_KEY
(named "Descriptions"). Since registry enumeration doesn't guarantee
any particular sort order, we should continue to search for matching
children rather than aborting the search.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
---
 net/tap-win32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 4e2fa55..5e5d6db 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -356,7 +356,8 @@ static int get_device_guid(
                 &len);
 
             if (status != ERROR_SUCCESS || name_type != REG_SZ) {
-                    return -1;
+                ++i;
+                continue;
             }
             else {
                 if (is_tap_win32_dev(enum_name)) {
-- 
2.5.3

[Qemu-devel] [PATCH v2 2/2] tap-win32: disable broken async write path

[Andrew Baumann]

The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect
assumptions about the behaviour of the WriteFile API for overlapped
file handles. First, WriteFile does not update the
lpNumberOfBytesWritten parameter when the write completes
asynchronously (the number of bytes written is known only when the
operation completes). Second, the buffer shouldn't be touched (or
freed) until the operation completes. This led to at least one bug
where tap_win32_write returned zero bytes written, which in turn
caused further writes ("receives") to be disabled for that device.

This change disables the asynchronous write path, while keeping most
of the code around in case someone sees value in resurrecting it. It
also adds some conditional debug output, similar to the read path.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
Version 2 changes:
 * pass &write_size to initial WriteFile call; we need to know this in
   case it ever completes synchronously
 * avoid a needless cast between LPVOID and LPTSTR in debug print

Overall, it's probably still cleaner to rip out the async write code,
but that's a job for another day.

net/tap-win32.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 5e5d6db..7fddb20 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -77,7 +77,12 @@
 
 //#define DEBUG_TAP_WIN32
 
-#define TUN_ASYNCHRONOUS_WRITES 1
+/* FIXME: The asynch write path appears to be broken at
+ * present. WriteFile() ignores the lpNumberOfBytesWritten parameter
+ * for overlapped writes, with the result we return zero bytes sent,
+ * and after handling a single packet, receive is disabled for this
+ * interface. */
+/* #define TUN_ASYNCHRONOUS_WRITES 1 */
 
 #define TUN_BUFFER_SIZE 1560
 #define TUN_MAX_BUFFER_COUNT 32
@@ -461,27 +466,48 @@ static int tap_win32_write(tap_win32_overlapped_t *overlapped,
     BOOL result;
     DWORD error;
 
+#ifdef TUN_ASYNCHRONOUS_WRITES
     result = GetOverlappedResult( overlapped->handle, &overlapped->write_overlapped,
                                   &write_size, FALSE);
 
     if (!result && GetLastError() == ERROR_IO_INCOMPLETE)
         WaitForSingleObject(overlapped->write_event, INFINITE);
+#endif
 
     result = WriteFile(overlapped->handle, buffer, size,
                        &write_size, &overlapped->write_overlapped);
 
+#ifdef TUN_ASYNCHRONOUS_WRITES
+    /* FIXME: we can't sensibly set write_size here, without waiting
+     * for the IO to complete! Moreover, we can't return zero,
+     * because that will disable receive on this interface, and we
+     * also can't assume it will succeed and return the full size,
+     * because that will result in the buffer being reclaimed while
+     * the IO is in progress. */
+#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES.
+#else /* !TUN_ASYNCHRONOUS_WRITES */
     if (!result) {
-        switch (error = GetLastError())
-        {
-        case ERROR_IO_PENDING:
-#ifndef TUN_ASYNCHRONOUS_WRITES
-            WaitForSingleObject(overlapped->write_event, INFINITE);
-#endif
-            break;
-        default:
-            return -1;
+        error = GetLastError();
+        if (error == ERROR_IO_PENDING) {
+            result = GetOverlappedResult(overlapped->handle,
+                                         &overlapped->write_overlapped,
+                                         &write_size, TRUE);
         }
     }
+#endif
+
+    if (!result) {
+#ifdef DEBUG_TAP_WIN32
+        LPTSTR msgbuf;
+        error = GetLastError();
+        FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM,
+                      NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                      &msgbuf, 0, NULL);
+        fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, msgbuf);
+        LocalFree(msgbuf);
+#endif
+        return 0;
+    }
 
     return write_size;
 }
-- 
2.5.3

Re: [Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Andrew Baumann]

Ping?

Jason, I know you planned to leave this for a few days... I just wanted to make sure it isn't forgotten :)

Thanks,
Andrew

> -----Original Message-----
> From: Andrew Baumann
> Sent: Wednesday, 18 November 2015 11:45
> To: qemu-devel@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>; Stefan Weil <sw@weilnetz.de>;
> Andrew Baumann <Andrew.Baumann@microsoft.com>
> Subject: [PATCH v2 0/2] net/tap-win32 bugfixes
> 
> Minor revisions to the second patch in the series.
> 
> Cheers,
> Andrew
> 
> Andrew Baumann (2):
>   tap-win32: skip unexpected nodes during registry enumeration
>   tap-win32: disable broken async write path
> 
>  net/tap-win32.c | 49 ++++++++++++++++++++++++++++++++++++++-------
> ----
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> --
> 2.5.3

Re: [Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Jason Wang]

On 11/25/2015 07:36 AM, Andrew Baumann wrote:
> Ping?
>
> Jason, I know you planned to leave this for a few days... I just wanted to make sure it isn't forgotten :)
>
> Thanks,
> Andrew

Thanks for working on this. I would expect Stefan to review v2 (since he
reviewed v1).

Stefan, could you please review v2?

>> -----Original Message-----
>> From: Andrew Baumann
>> Sent: Wednesday, 18 November 2015 11:45
>> To: qemu-devel@nongnu.org
>> Cc: Jason Wang <jasowang@redhat.com>; Stefan Weil <sw@weilnetz.de>;
>> Andrew Baumann <Andrew.Baumann@microsoft.com>
>> Subject: [PATCH v2 0/2] net/tap-win32 bugfixes
>>
>> Minor revisions to the second patch in the series.
>>
>> Cheers,
>> Andrew
>>
>> Andrew Baumann (2):
>>   tap-win32: skip unexpected nodes during registry enumeration
>>   tap-win32: disable broken async write path
>>
>>  net/tap-win32.c | 49 ++++++++++++++++++++++++++++++++++++++-------
>> ----
>>  1 file changed, 38 insertions(+), 11 deletions(-)
>>
>> --
>> 2.5.3

Re: [Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Stefan Weil]

Am 25.11.2015 um 04:01 schrieb Jason Wang:
> 
> 
> On 11/25/2015 07:36 AM, Andrew Baumann wrote:
>> Ping?
>>
>> Jason, I know you planned to leave this for a few days... I just wanted to make sure it isn't forgotten :)
>>
>> Thanks,
>> Andrew
> 
> Thanks for working on this. I would expect Stefan to review v2 (since he
> reviewed v1).
> 
> Stefan, could you please review v2?


Hi Jason,

patch 1/2 already contains my Reviewed-by, but I cannot say
much about patch 2/2, simply because I cannot test it.

As the changes in 2/2 look plausible, I suggest nevertheless
to apply both patches for QEMU 2.5. What would be the correct
way to indicate this? Acked-by: Stefan Weil <sw@weilnetz.de>?
Feel free to add it to patch 2/2 if needed.

Kind regards,
Stefan

Re: [Qemu-devel] [PATCH v2 0/2] net/tap-win32 bugfixes

[Jason Wang]

On 11/25/2015 02:13 PM, Stefan Weil wrote:
> Am 25.11.2015 um 04:01 schrieb Jason Wang:
>>
>> On 11/25/2015 07:36 AM, Andrew Baumann wrote:
>>> Ping?
>>>
>>> Jason, I know you planned to leave this for a few days... I just wanted to make sure it isn't forgotten :)
>>>
>>> Thanks,
>>> Andrew
>> Thanks for working on this. I would expect Stefan to review v2 (since he
>> reviewed v1).
>>
>> Stefan, could you please review v2?
>
> Hi Jason,
>
> patch 1/2 already contains my Reviewed-by, but I cannot say
> much about patch 2/2, simply because I cannot test it.
>
> As the changes in 2/2 look plausible, I suggest nevertheless
> to apply both patches for QEMU 2.5. What would be the correct
> way to indicate this? Acked-by: Stefan Weil <sw@weilnetz.de>?
> Feel free to add it to patch 2/2 if needed.
>
> Kind regards,
> Stefan

Thanks for the reviewing.

Apply the series for 2.5 in my -net tree.