Re: [RFC PATCH v2 1/9] net: Pad short frames to minimum size before send from SLiRP/TAP

[Philippe Mathieu-Daudé]

On 3/3/21 10:21 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> The minimum Ethernet frame length is 60 bytes. For short frames with
> smaller length like ARP packets (only 42 bytes), on a real world NIC
> it can choose either padding its length to the minimum required 60
> bytes, or sending it out directly to the wire. Such behavior can be
> hardcoded or controled by a register bit. Similarly on the receive
> path, NICs can choose either dropping such short frames directly or
> handing them over to software to handle.
> 
> On the other hand, for the network backends SLiRP/TAP, they don't
> expose a way to control the short frame behavior. As of today they
> just send/receive data from/to the other end connected to them,
> which means any sized packet is acceptable. So they can send and
> receive short frames without any problem. It is observed that ARP
> packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send
> these ARP packets to the other end which might be a NIC model that
> does not allow short frames to pass through.
> 
> To provide better compatibility, for packets sent from SLiRP/TAP, we
> change to pad short frames before sending it out to the other end.
> This ensures SLiRP/TAP as an Ethernet sender do not violate the spec.
> But with this change, the behavior of dropping short frames in the
> NIC model cannot be emulated because it always receives a packet that
> is spec complaint. The capability of sending short frames from NIC
> models are still supported and short frames can still pass through
> SLiRP/TAP interfaces.
> 
> This commit should be able to fix the issue as reported with some
> NIC models before, that ARP requests get dropped, preventing the
> guest from becoming visible on the network. It was workarounded in
> these NIC models on the receive path, that when a short frame is
> received, it is padded up to 60 bytes.
> 
> The following 2 commits seem to be the one to workaround this issue
> in e1000 and vmxenet3 before, and should probably be reverted.
> 
>   commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
>   commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - only pad short frames for SLiRP/TAP interfaces
> 
>  include/net/eth.h |  1 +
>  net/net.c         | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/net/eth.h b/include/net/eth.h
> index 0671be6..7c825ec 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -31,6 +31,7 @@
>  
>  #define ETH_ALEN 6
>  #define ETH_HLEN 14
> +#define ETH_ZLEN 60     /* Min. octets in frame sans FCS */
>  
>  struct eth_header {
>      uint8_t  h_dest[ETH_ALEN];   /* destination eth addr */
> diff --git a/net/net.c b/net/net.c
> index 32d71c1..27c3b25 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -638,6 +638,7 @@ static ssize_t 
> qemu_send_packet_async_with_flags(NetClientState *sender,
>                                                   NetPacketSent *sent_cb)
>  {
>      NetQueue *queue;
> +    uint8_t min_buf[ETH_ZLEN];
>      int ret;
>  
>  #ifdef DEBUG_NET
> @@ -649,6 +650,17 @@ static ssize_t 
> qemu_send_packet_async_with_flags(NetClientState *sender,
>          return size;
>      }
>  
> +    /* Pad to minimum Ethernet frame length for SLiRP and TAP */
> +    if (sender->info->type == NET_CLIENT_DRIVER_USER ||
> +        sender->info->type == NET_CLIENT_DRIVER_TAP) {
> +        if (size < ETH_ZLEN) {
> +            memcpy(min_buf, buf, size);
> +            memset(&min_buf[size], 0, ETH_ZLEN - size);
> +            buf = min_buf;
> +            size = ETH_ZLEN;
> +        }

We can have zero-copy by using a static zeroed buf and rewrite
this function to call the _iov() equivalents with a pair of
struct iovec.

> +    }
> +
>      /* Let filters handle the packet first */
>      ret = filter_receive(sender, NET_FILTER_DIRECTION_TX,
>                           sender, flags, buf, size, sent_cb);
>