[v3] net/vmnet: Pad short Ethernet frames

[PATCH v3] net/vmnet: Pad short Ethernet frames

Posted by William Hooper 1 year, 3 months ago

At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the
host's ARP replies, to the minimum size (60 bytes before the frame check
sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device
drivers may drop them with "frame too short" errors.

This patch calls eth_pad_short_frame() to add padding, as in net/tap.c
and net/slirp.c. Thanks to Bin Meng, Philippe Mathieu-Daudé, and Phil
Dennis-Jordan for reviewing earlier versions.

Signed-off-by: William Hooper <wsh@wshooper.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058
Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 net/vmnet-common.m | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/net/vmnet-common.m b/net/vmnet-common.m
index 30c4e53c13..4b7e330c05 100644
--- a/net/vmnet-common.m
+++ b/net/vmnet-common.m
@@ -18,6 +18,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "sysemu/runstate.h"
+#include "net/eth.h"
 
 #include <vmnet/vmnet.h>
 #include <dispatch/dispatch.h>
@@ -147,10 +148,26 @@ static int vmnet_read_packets(VmnetState *s)
  */
 static void vmnet_write_packets_to_qemu(VmnetState *s)
 {
+    uint8_t *pkt;
+    size_t pktsz;
+    uint8_t min_pkt[ETH_ZLEN];
+    size_t min_pktsz;
+    ssize_t size;
+
     while (s->packets_send_current_pos < s->packets_send_end_pos) {
-        ssize_t size = qemu_send_packet_async(&s->nc,
-                                      s->iov_buf[s->packets_send_current_pos].iov_base,
-                                      s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
+        pkt = s->iov_buf[s->packets_send_current_pos].iov_base;
+        pktsz = s->packets_buf[s->packets_send_current_pos].vm_pkt_size;
+
+        if (net_peer_needs_padding(&s->nc)) {
+            min_pktsz = sizeof(min_pkt);
+
+            if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) {
+                pkt = min_pkt;
+                pktsz = min_pktsz;
+            }
+        }
+
+        size = qemu_send_packet_async(&s->nc, pkt, pktsz,
                                       vmnet_send_completed);
 
         if (size == 0) {

Ping (2): [PATCH v3] net/vmnet: Pad short Ethernet frames

Posted by William Hooper 1 year, 1 month ago

On Sat, Nov 2, 2024 at 1:56 PM William Hooper <wsh@wshooper.org> wrote:
> At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the
> host's ARP replies, to the minimum size (60 bytes before the frame check
> sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device
> drivers may drop them with "frame too short" errors.
>
> This patch calls eth_pad_short_frame() to add padding, as in net/tap.c
> and net/slirp.c. Thanks to Bin Meng, Philippe Mathieu-Daudé, and Phil
> Dennis-Jordan for reviewing earlier versions.
>
> Signed-off-by: William Hooper <wsh@wshooper.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>  net/vmnet-common.m | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> index 30c4e53c13..4b7e330c05 100644
> --- a/net/vmnet-common.m
> +++ b/net/vmnet-common.m
> @@ -18,6 +18,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "sysemu/runstate.h"
> +#include "net/eth.h"
>
>  #include <vmnet/vmnet.h>
>  #include <dispatch/dispatch.h>
> @@ -147,10 +148,26 @@ static int vmnet_read_packets(VmnetState *s)
>   */
>  static void vmnet_write_packets_to_qemu(VmnetState *s)
>  {
> +    uint8_t *pkt;
> +    size_t pktsz;
> +    uint8_t min_pkt[ETH_ZLEN];
> +    size_t min_pktsz;
> +    ssize_t size;
> +
>      while (s->packets_send_current_pos < s->packets_send_end_pos) {
> -        ssize_t size = qemu_send_packet_async(&s->nc,
> -                                      s->iov_buf[s->packets_send_current_pos].iov_base,
> -                                      s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> +        pkt = s->iov_buf[s->packets_send_current_pos].iov_base;
> +        pktsz = s->packets_buf[s->packets_send_current_pos].vm_pkt_size;
> +
> +        if (net_peer_needs_padding(&s->nc)) {
> +            min_pktsz = sizeof(min_pkt);
> +
> +            if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) {
> +                pkt = min_pkt;
> +                pktsz = min_pktsz;
> +            }
> +        }
> +
> +        size = qemu_send_packet_async(&s->nc, pkt, pktsz,
>                                        vmnet_send_completed);
>
>          if (size == 0) {

Ping?

https://patchew.org/QEMU/20241102205653.30476-1-wsh@wshooper.org/

Re: Ping (2): [PATCH v3] net/vmnet: Pad short Ethernet frames

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

Hi William,

On 30/12/24 22:05, William Hooper wrote:
> On Sat, Nov 2, 2024 at 1:56 PM William Hooper <wsh@wshooper.org> wrote:
>> At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the
>> host's ARP replies, to the minimum size (60 bytes before the frame check
>> sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device
>> drivers may drop them with "frame too short" errors.
>>
>> This patch calls eth_pad_short_frame() to add padding, as in net/tap.c
>> and net/slirp.c. Thanks to Bin Meng, Philippe Mathieu-Daudé, and Phil
>> Dennis-Jordan for reviewing earlier versions.
>>
>> Signed-off-by: William Hooper <wsh@wshooper.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058
>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>   net/vmnet-common.m | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)

> Ping?

We missed this patch because:

1/ maintainers were not Cc'ed (Jason now added)

$ ./scripts/get_maintainer.pl -f net/vmnet-common.m
Jason Wang <jasowang@redhat.com> (maintainer:Network device ba...)
"Philippe Mathieu-Daudé" <philmd@linaro.org> (odd fixer:Darwin (macOS, iOS))

2/ it is buried as a reply on a previous patch, see [*]:

"Send each new revision as a new top-level thread, rather than burying
  it in-reply-to an earlier revision, as many reviewers are not looking
  inside deep threads for new patches."

Anyway, now queued!

Regards,

Phil.

[*] 
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag

Ping: [PATCH v3] net/vmnet: Pad short Ethernet frames

Posted by William Hooper 1 year, 2 months ago

On Sat, Nov 2, 2024 at 1:56 PM William Hooper <wsh@wshooper.org> wrote:
> At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the
> host's ARP replies, to the minimum size (60 bytes before the frame check
> sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device
> drivers may drop them with "frame too short" errors.
>
> This patch calls eth_pad_short_frame() to add padding, as in net/tap.c
> and net/slirp.c. Thanks to Bin Meng, Philippe Mathieu-Daudé, and Phil
> Dennis-Jordan for reviewing earlier versions.
>
> Signed-off-by: William Hooper <wsh@wshooper.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>  net/vmnet-common.m | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/net/vmnet-common.m b/net/vmnet-common.m
> index 30c4e53c13..4b7e330c05 100644
> --- a/net/vmnet-common.m
> +++ b/net/vmnet-common.m
> @@ -18,6 +18,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "sysemu/runstate.h"
> +#include "net/eth.h"
>
>  #include <vmnet/vmnet.h>
>  #include <dispatch/dispatch.h>
> @@ -147,10 +148,26 @@ static int vmnet_read_packets(VmnetState *s)
>   */
>  static void vmnet_write_packets_to_qemu(VmnetState *s)
>  {
> +    uint8_t *pkt;
> +    size_t pktsz;
> +    uint8_t min_pkt[ETH_ZLEN];
> +    size_t min_pktsz;
> +    ssize_t size;
> +
>      while (s->packets_send_current_pos < s->packets_send_end_pos) {
> -        ssize_t size = qemu_send_packet_async(&s->nc,
> -                                      s->iov_buf[s->packets_send_current_pos].iov_base,
> -                                      s->packets_buf[s->packets_send_current_pos].vm_pkt_size,
> +        pkt = s->iov_buf[s->packets_send_current_pos].iov_base;
> +        pktsz = s->packets_buf[s->packets_send_current_pos].vm_pkt_size;
> +
> +        if (net_peer_needs_padding(&s->nc)) {
> +            min_pktsz = sizeof(min_pkt);
> +
> +            if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) {
> +                pkt = min_pkt;
> +                pktsz = min_pktsz;
> +            }
> +        }
> +
> +        size = qemu_send_packet_async(&s->nc, pkt, pktsz,
>                                        vmnet_send_completed);
>
>          if (size == 0) {

Ping?

https://patchew.org/QEMU/20241102205653.30476-1-wsh@wshooper.org/