netdev - Re: [PATCH v4 net-next] net: ftmac100: support mtu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221019165516.sgoddwmdx6srmh5e@skbuf>
Date:   Wed, 19 Oct 2022 19:55:16 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sergei Antonov <saproj@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, andrew@...n.ch
Subject: Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> The ftmac100 controller considers some packets FTL (frame
> too long) and drops them. An example of a dropped packet:
> 6 bytes - dst MAC
> 6 bytes - src MAC
> 2 bytes - EtherType IPv4 (0800)
> 1504 bytes - IPv4 packet

Why do you insist writing these confusing messages?

It's pretty straightforward. If the FTMAC100 is used as a DSA master,
then it is expected that frames which are MTU sized on the wire facing
the external switch port (1500 octets in L2 payload, plus L2 header)
also get a DSA tag when seen by the host port.

This extra tag increases the length of the packet as the host port sees
it, and the FTMAC100 is not prepared to handle frames whose length
exceeds 1518 octets (including FCS) at all.

Only a minimal rework is needed to support this configuration. Since
MTU-sized DSA-tagged frames still fit within a single buffer (RX_BUF_SIZE),
we just need to optimize the resource management rather than implement
multi buffer RX.

In ndo_mtu_change, we toggle the FTMAC100_MACCR_RX_FTL bit to tell the
hardware to drop (or not) frames with an L2 payload length larger than
1500. And since setting this bit, and accepting frames with the FTL bit
in the BD status, exposes us to the danger of receiving multi-buffer
frames (which we still do not support), we need to replace the BUG_ON()
with a proper call to ftmac100_rx_drop_packet(). We need to replicate
the MACCR configuration in ftmac100_start_hw() as well, since there is a
hardware reset there which clears previous settings.

The advantage of dynamically changing FTMAC100_MACCR_RX_FTL is that when
dev->mtu is at the default value of 1500, large frames are automatically
dropped in hardware and we do not spend CPU cycles dropping them.

> Do the following to let the driver receive these packets.
> Set FTMAC100_MACCR_RX_FTL when mtu>1500 in the MAC Control Register.
> For received packets marked with FTMAC100_RXDES0_FTL check if packet
> length (with FCS excluded) is within expected limits, that is not
> greater than netdev->mtu + 14 (Ethernet headers). Otherwise trigger
> an error.
> 
> Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")

Please drop the Fixes: tag, I thought we agreed this patch wouldn't get
backported, since it does not fix any bug in this driver.

> Signed-off-by: Sergei Antonov <saproj@...il.com>
> Suggested-by: Vladimir Oltean <olteanv@...il.com>

You essentially did nothing from what I suggested. Please remove this
tag, it is misleading.

> ---
>  drivers/net/ethernet/faraday/ftmac100.c | 29 ++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
> index d95d78230828..f89b53845f21 100644
> --- a/drivers/net/ethernet/faraday/ftmac100.c
> +++ b/drivers/net/ethernet/faraday/ftmac100.c
> @@ -159,6 +159,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
>  static int ftmac100_start_hw(struct ftmac100 *priv)
>  {
>  	struct net_device *netdev = priv->netdev;
> +	unsigned int maccr;
>  
>  	if (ftmac100_reset(priv))
>  		return -EIO;
> @@ -175,7 +176,20 @@ static int ftmac100_start_hw(struct ftmac100 *priv)
>  
>  	ftmac100_set_mac(priv, netdev->dev_addr);
>  
> -	iowrite32(MACCR_ENABLE_ALL, priv->base + FTMAC100_OFFSET_MACCR);
> +	maccr = MACCR_ENABLE_ALL;
> +
> +	/* We have to set FTMAC100_MACCR_RX_FTL in case MTU > 1500
> +	 * and do extra length check in ftmac100_rx_packet_error().
> +	 * Otherwise the controller silently drops these packets.
> +	 *
> +	 * When the MTU of the interface is standard 1500, rely on
> +	 * the controller's functionality to drop too long packets
> +	 * and save some CPU time.
> +	 */
> +	if (netdev->mtu > 1500)
> +		maccr |= FTMAC100_MACCR_RX_FTL;

It is expected that ndo_change_mtu() handles this as well, so that the
MTU change takes effect right away even if the device is open.

> +
> +	iowrite32(maccr, priv->base + FTMAC100_OFFSET_MACCR);
>  	return 0;
>  }
>  
> @@ -337,9 +351,18 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  		error = true;
>  	}
>  
> -	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
> +	/* If the frame-too-long flag FTMAC100_RXDES0_FTL is set, check
> +	 * if ftmac100_rxdes_frame_length(rxdes) exceeds the currently
> +	 * set MTU plus ETH_HLEN.
> +	 * The controller would set FTMAC100_RXDES0_FTL for all incoming
> +	 * frames longer than 1518 (includeing FCS) in the presense of
> +	 * FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> +	 */
> +	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) &&
> +		     ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN)) {

You didn't explain why you can't drop this altogether?

>  		if (net_ratelimit())
> -			netdev_info(netdev, "rx frame too long\n");
> +			netdev_info(netdev, "rx frame too long (%u)\n",
> +				    ftmac100_rxdes_frame_length(rxdes));
>  
>  		netdev->stats.rx_length_errors++;
>  		error = true;
> -- 
> 2.34.1
>