Message-ID: <379c5499d1be4f73a6175385d3345a68@AcuMS.aculab.com>
Date:   Thu, 13 Oct 2022 04:24:39 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Sergei Antonov' <saproj@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>
Subject: RE: [PATCH v2 net] net: ftmac100: do not reject packets bigger than
 1514

From: Sergei Antonov
> Sent: 12 October 2022 17:43
> 
> On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Sergei Antonov
> > > Sent: 12 October 2022 16:38
> > >
> > > Despite the datasheet [1] saying the controller should allow incoming
> > > packets of length >=1518, it only allows packets of length <=1514.

Actually I bet the datasheet is correct.
The length check is probably done before the CRC is discarded.

...
> > Although traditionally it was 1514+crc.
> > An extra 4 byte header is now allowed.
> > There is also the usefulness of supporting full length frames
> > with a PPPoE header.
> >
> > Whether it actually makes sense to round up the receive buffer
> > size and associated max frame length to 1536 (cache line aligned)
> > is another matter (probably 1534 for 4n+2 alignment).
> >
> > > Since 1518 is a standard Ethernet maximum frame size, and it can
> > > easily be encountered (in SSH for example), fix this behavior:
> > >
> > > * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> >
> > What does that do?
> 
> If FTMAC100_MACCR_RX_FTL is not set:
>   the driver does not receive the "long" packet at all. Looks like the
> controller discards the packet without bothering the driver.
> If FTMAC100_MACCR_RX_FTL is set:
>   the driver receives the "long" packet marked by the
> FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
> driver (before my patch).

There are other problems here.
Where does the extra data actually get written to?
What happens to very long packets?

I'm guessing the hardware has a reasonable interface where
there is a 'ring' of receive buffer descriptors.
If a frame is longer than a buffer the hardware will write
the frame to multiple buffers.

However if each buffer is long enough for a normal frame
and the hardware discards/truncates overlong frames then
the driver can assume there are no continuations.
(I used to use an array of 128 buffers of 512 bytes and
always copy the receive data - a single word aligned copy
unless a long frame crossed the ring end.)

It looks like the FTL bit actually controls whether
overlong frames are discarded or truncated.
(There may be another option to either set the frame
length or disable the feature completely.)

Now you might get away with packets that are only 4 bytes
overlong (one VLAN header) because the hardware always
writes the full received CRC into the buffer.
So when it discards a 1515-1518 frame the extra bytes
are from the frame.
If that is true it deserves a comment.

OTOH if it carries on writing into the rx ring buffer
then it might also 'overflow' into the next ring entry.
Indeed a long enough frame will fill the entire ring
(you'll need a buggy ethernet hub/switch of a 10/100M
HDX network).

You really do need to check whether it detects CRC errors
on overlong frames. If it (mostly) stops processing at
1518 bytes (inc crc) then it may not.
Also if the frame length field is (say) 16 bits then
a 64k+ frame will wrap the counter.
Which means that the frame length may be unreliable
for overlong frames.
(The count might saturate.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists