Re: [PULL 02/15] hw/net: e1000: Remove the logic of padding short frames in the receive path

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 7 Jul 2023 at 10:06, Jason Wang <jasowang@redhat.com> wrote:
>
> From: Bin Meng <bmeng@tinylab.org>
>
> Now that we have implemented unified short frames padding in the
> QEMU networking codes, remove the same logic in the NIC codes.
>
> This actually reverts commit 78aeb23eded2d0b765bf9145c71f80025b568acd.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/e1000.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index aae5f0b..093c2d4 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -888,7 +888,6 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>      uint16_t vlan_special = 0;
>      uint8_t vlan_status = 0;
>      uint8_t min_buf[ETH_ZLEN];
> -    struct iovec min_iov;
>      uint8_t *filter_buf = iov->iov_base;
>      size_t size = iov_size(iov, iovcnt);
>      size_t iov_ofs = 0;
> @@ -905,15 +904,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>          return 0;
>      }
>
> -    /* Pad to minimum Ethernet frame length */
> -    if (size < sizeof(min_buf)) {
> -        iov_to_buf(iov, iovcnt, 0, min_buf, size);
> -        memset(&min_buf[size], 0, sizeof(min_buf) - size);
> -        min_iov.iov_base = filter_buf = min_buf;
> -        min_iov.iov_len = size = sizeof(min_buf);
> -        iovcnt = 1;
> -        iov = &min_iov;
> -    } else if (iov->iov_len < MAXIMUM_ETHERNET_HDR_LEN) {
> +    if (iov->iov_len < MAXIMUM_ETHERNET_HDR_LEN) {
>          /* This is very unlikely, but may happen. */
>          iov_to_buf(iov, iovcnt, 0, min_buf, MAXIMUM_ETHERNET_HDR_LEN);
>          filter_buf = min_buf;

Hi; I'm investigating https://gitlab.com/qemu-project/qemu/-/issues/3043
and part of the problem seems to be this commit.

The repro case puts the e1000 into loopback mode, and then makes
the e1000 send out a zero-sized packet, which the net/ code feeds
back into the e1000's receive path. This then falls over because
the code in e1000_receive_iov() is not expecting a zero length iov
and walks off the end of the iov. Before this code was removed,
we would have padded the packet out to the minimum frame length.

If the idea is that ethernet device models can now assume
the packet is not short, who is responsible for ensuring
this for cases like loopback? Is it a bug in the e1000
transmit path? Or should the net core code be padding?
Is there somewhere we can conveniently assert that we
really did do the padding so that other cases which got
missed at least assert rather than walking off buffers?

thanks
-- PMM