Message-ID: <f05f9dd9b39f42d18df0018c3596d866@AcuMS.aculab.com>
Date:   Wed, 12 Oct 2022 21:41:25 +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.
> >
> > Shouldn't that be <=1518 and <1518 ??
> 
> Oh, thanks for noticing. But still it should be slightly different:
> <= 1518 and <=1514
> Here is my test results of different packet sizes:
> packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
> (before my patch)
> packets of 1514 and less bytes did come

I had to double check the frames sizes, not written an ethernet driver
for nearly 30 years! There is a nice description that is 90% accurate
at https://en.wikipedia.org/wiki/Ethernet_frame

Without an 802.1Q tag (probably a VLAN tag?) the max frame has
1514 data bytes (inc mac addresses, but excl crc).
Unless you are using VLANs that should be the frame limit.
The IP+TCP is limited to the 1500 byte payload.

So if the sender is generating longer packets it is buggy!

...
> > > 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.

Right so the existing check for the flag being set could never happen.

> 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).
> 
> > Looks like it might cause 'Frame Too Long' packets be returned.
> > In which case should the code just have ignored it since
> > longer frames would be discarded completely??
> 
> Is there such a thing as a response packet which is sent in return to
> FTL packet? Did not know that. My testcases were SSH and SCP programs
> on Ubuntu 22 and they simply hang trying to connect to the ftmac100
> device - no retransmissions or retries with smaller frames happened.

Overlong frames should be discarded.
The sender might choose to do PMTU (path MTU) detection,
but probably doesn't unless a router is involved.

...
> > Do you need to read this value this early in the function?
> > Looks like it is only used when overlong packets are reported.
> 
> I decided to make a variable in order to use it twice:
> in the condition: "length > 1518"
> in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
> You are right saying it is not needed in most cases. Can we hope for
> the optimizer to postpone the initialization of 'length' till it is
> accessed?

Unlikely unless there are no function calls and no volatile
memory accesses.
IMHO just because you can assign a value on the declaration
(of a local) doesn't mean it is a good idea.
Better to move it nearer the use (unless it is used throughout
the function).

	David

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

Powered by blists - more mailing lists