[PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

As of today both slirp and tap networking do not pad short frames
(e.g.: an ARP packet) to the minimum frame size of 60 bytes.

If eTSEC is programmed to reject short frames, ARP requests will be
dropped, preventing the guest from becoming visible on the network.

The same issue was reported on e1000 and vmxenet3 before, see:

commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")

Ideally this should be fixed on the slirp/tap networking side to
pad short frames to the minimum frame length, but I am not sure
whether that's doable.

This commit changes to codes to ignore the RCTRL_RSF setting and
still allow receiving the short frame. The log message is updated
to mention the reject short frames functionality is unimplemented.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

RESEND using correct email address

 hw/net/fsl_etsec/rings.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 121415a..503b4d3 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
         return -1;
     }
 
+    /*
+     * Both slirp and tap networking do not pad short frames
+     * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
+     *
+     * If eTSEC is programmed to reject short frames, ARP requests
+     * will be dropped, preventing the guest from becoming visible
+     * on the network.
+     */
     if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
         /* CRC is not in the packet yet, so short frame is below 60 bytes */
-        RING_DEBUG("%s: Drop short frame\n", __func__);
-        return -1;
+        RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
     }
 
     rx_init_frame(etsec, buf, size);
-- 
2.7.4

Re: [PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[Peter Maydell]

On Mon, 8 Feb 2021 at 14:53, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> As of today both slirp and tap networking do not pad short frames
> (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
>
> If eTSEC is programmed to reject short frames, ARP requests will be
> dropped, preventing the guest from becoming visible on the network.
>
> The same issue was reported on e1000 and vmxenet3 before, see:
>
> commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")

How a short frame should be handled is ethernet device specific:
what is correct for one device model doesn't necessarily apply
to another.

> Ideally this should be fixed on the slirp/tap networking side to
> pad short frames to the minimum frame length, but I am not sure
> whether that's doable.

It would be useful to investigate further exactly where these
short frames are coming from. If one guest is sending out short
frames, or we are doing tap networking and get a genuine short
frame from some external host then we should pass them to the
guest as short frames; if QEMU itself is generating frames (eg
from the 'fake' hosts in usermode networking) then it should be
generating valid frames, not bogus ones, and we should fix whatever
bit of code that is.

> This commit changes to codes to ignore the RCTRL_RSF setting and
> still allow receiving the short frame. The log message is updated
> to mention the reject short frames functionality is unimplemented.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
> RESEND using correct email address
>
>  hw/net/fsl_etsec/rings.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 121415a..503b4d3 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
>          return -1;
>      }
>
> +    /*
> +     * Both slirp and tap networking do not pad short frames
> +     * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> +     *
> +     * If eTSEC is programmed to reject short frames, ARP requests
> +     * will be dropped, preventing the guest from becoming visible
> +     * on the network.
> +     */
>      if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
>          /* CRC is not in the packet yet, so short frame is below 60 bytes */
> -        RING_DEBUG("%s: Drop short frame\n", __func__);
> -        return -1;
> +        RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
>      }

This doesn't look right. If the guest programs the device to
reject frames less than 60 bytes and then expects to recieve a
frame that's less than 60 bytes, that's a guest bug. If QEMU
itself is generating packets to send and they're short that sounds
like a bug elsewhere in QEMU.

But I think the actual problem here is much simpler:
the datasheet says
# RSF: Receive short frame mode. When set, enables the reception of
# frames shorter than 64 bytes. [...]
#    0 Ethernet frames less than 64B in length are silently dropped
#    1 Frames less than 64B are accepted upon a DA match
(https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf chapter 14)

whereas the QEMU code is doing the reverse: dropping short
packets if the bit is 1.

If you fix this bug by reversing the sense of the test on the
RSF bit, does it make your guest happier ?

thanks
-- PMM

Re: [PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[Bin Meng]

Cc'ing libSLiRP

Hi Peter,

On Tue, Feb 9, 2021 at 12:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 8 Feb 2021 at 14:53, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > As of today both slirp and tap networking do not pad short frames
> > (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> >
> > If eTSEC is programmed to reject short frames, ARP requests will be
> > dropped, preventing the guest from becoming visible on the network.
> >
> > The same issue was reported on e1000 and vmxenet3 before, see:
> >
> > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
>
> How a short frame should be handled is ethernet device specific:
> what is correct for one device model doesn't necessarily apply
> to another.

I digged some history about the above 2 commits and they are the same
issue caused by slirp and tap networking, and workarounded in the
ethernet controller models.

>
> > Ideally this should be fixed on the slirp/tap networking side to
> > pad short frames to the minimum frame length, but I am not sure
> > whether that's doable.
>
> It would be useful to investigate further exactly where these
> short frames are coming from. If one guest is sending out short
> frames, or we are doing tap networking and get a genuine short
> frame from some external host then we should pass them to the
> guest as short frames; if QEMU itself is generating frames (eg
> from the 'fake' hosts in usermode networking) then it should be
> generating valid frames, not bogus ones, and we should fix whatever
> bit of code that is.

From what I can tell it's the QEMU networking codes that generate such
short frames.

However it looks no one has ever attempted to fix that in the QEMU
networking, instead the ethernet controller models are patched in the
*receive* path, which is to pad such short frames to 60 bytes in e1000
and vmxnet3.

>
> > This commit changes to codes to ignore the RCTRL_RSF setting and
> > still allow receiving the short frame. The log message is updated
> > to mention the reject short frames functionality is unimplemented.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> > RESEND using correct email address
> >
> >  hw/net/fsl_etsec/rings.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> > index 121415a..503b4d3 100644
> > --- a/hw/net/fsl_etsec/rings.c
> > +++ b/hw/net/fsl_etsec/rings.c
> > @@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> >          return -1;
> >      }
> >
> > +    /*
> > +     * Both slirp and tap networking do not pad short frames
> > +     * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > +     *
> > +     * If eTSEC is programmed to reject short frames, ARP requests
> > +     * will be dropped, preventing the guest from becoming visible
> > +     * on the network.
> > +     */
> >      if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> >          /* CRC is not in the packet yet, so short frame is below 60 bytes */
> > -        RING_DEBUG("%s: Drop short frame\n", __func__);
> > -        return -1;
> > +        RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> >      }
>
> This doesn't look right. If the guest programs the device to
> reject frames less than 60 bytes and then expects to recieve a
> frame that's less than 60 bytes, that's a guest bug. If QEMU
> itself is generating packets to send and they're short that sounds
> like a bug elsewhere in QEMU.
>
> But I think the actual problem here is much simpler:
> the datasheet says
> # RSF: Receive short frame mode. When set, enables the reception of
> # frames shorter than 64 bytes. [...]
> #    0 Ethernet frames less than 64B in length are silently dropped
> #    1 Frames less than 64B are accepted upon a DA match
> (https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf chapter 14)
>
> whereas the QEMU code is doing the reverse: dropping short
> packets if the bit is 1.

Yes, that's correct. I will revise my commit message in v2.

>
> If you fix this bug by reversing the sense of the test on the
> RSF bit, does it make your guest happier ?

Yes.

Regards,
Bin

Re: [PATCH RESEND] hw/net: fsl_etsec: Do not reject short frames

[Bin Meng]

On Tue, Feb 9, 2021 at 8:06 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Cc'ing libSLiRP

I am not sure whether my reply arrived to libSLiRP as I did not
subscribe to that list ...

>
> Hi Peter,
>
> On Tue, Feb 9, 2021 at 12:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 8 Feb 2021 at 14:53, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > As of today both slirp and tap networking do not pad short frames
> > > (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > >
> > > If eTSEC is programmed to reject short frames, ARP requests will be
> > > dropped, preventing the guest from becoming visible on the network.
> > >
> > > The same issue was reported on e1000 and vmxenet3 before, see:
> > >
> > > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> > > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
> >
> > How a short frame should be handled is ethernet device specific:
> > what is correct for one device model doesn't necessarily apply
> > to another.
>
> I digged some history about the above 2 commits and they are the same
> issue caused by slirp and tap networking, and workarounded in the
> ethernet controller models.
>
> >
> > > Ideally this should be fixed on the slirp/tap networking side to
> > > pad short frames to the minimum frame length, but I am not sure
> > > whether that's doable.
> >
> > It would be useful to investigate further exactly where these
> > short frames are coming from. If one guest is sending out short
> > frames, or we are doing tap networking and get a genuine short
> > frame from some external host then we should pass them to the
> > guest as short frames; if QEMU itself is generating frames (eg
> > from the 'fake' hosts in usermode networking) then it should be
> > generating valid frames, not bogus ones, and we should fix whatever
> > bit of code that is.
>
> From what I can tell it's the QEMU networking codes that generate such
> short frames.
>
> However it looks no one has ever attempted to fix that in the QEMU
> networking, instead the ethernet controller models are patched in the
> *receive* path, which is to pad such short frames to 60 bytes in e1000
> and vmxnet3.
>
> >
> > > This commit changes to codes to ignore the RCTRL_RSF setting and
> > > still allow receiving the short frame. The log message is updated
> > > to mention the reject short frames functionality is unimplemented.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > > RESEND using correct email address
> > >
> > >  hw/net/fsl_etsec/rings.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> > > index 121415a..503b4d3 100644
> > > --- a/hw/net/fsl_etsec/rings.c
> > > +++ b/hw/net/fsl_etsec/rings.c
> > > @@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> > >          return -1;
> > >      }
> > >
> > > +    /*
> > > +     * Both slirp and tap networking do not pad short frames
> > > +     * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > > +     *
> > > +     * If eTSEC is programmed to reject short frames, ARP requests
> > > +     * will be dropped, preventing the guest from becoming visible
> > > +     * on the network.
> > > +     */
> > >      if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > >          /* CRC is not in the packet yet, so short frame is below 60 bytes */
> > > -        RING_DEBUG("%s: Drop short frame\n", __func__);
> > > -        return -1;
> > > +        RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> > >      }
> >
> > This doesn't look right. If the guest programs the device to
> > reject frames less than 60 bytes and then expects to recieve a
> > frame that's less than 60 bytes, that's a guest bug. If QEMU
> > itself is generating packets to send and they're short that sounds
> > like a bug elsewhere in QEMU.

Could anyone familiar with the QEMU networking have a look at the
implementations to solve this short frame issue once and for all?

Regards,
Bin