netdev - Re: [PATCH v3 net-next 3/8] net: dsa: configure the MTU for switch ports

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: <20200327000625.GJ3819@lunn.ch>
Date:   Fri, 27 Mar 2020 01:06:25 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     f.fainelli@...il.com, vivien.didelot@...il.com,
        davem@...emloft.net, jakub.kicinski@...ronome.com,
        murali.policharla@...adcom.com, stephen@...workplumber.org,
        jiri@...nulli.us, idosch@...sch.org, kuba@...nel.org,
        nikolay@...ulusnetworks.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 3/8] net: dsa: configure the MTU for switch
 ports

> -static void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
> -{
> -	unsigned int mtu = ETH_DATA_LEN + cpu_dp->tag_ops->overhead;
> -	int err;
> -
> -	rtnl_lock();
> -	if (mtu <= dev->max_mtu) {
> -		err = dev_set_mtu(dev, mtu);
> -		if (err)
> -			netdev_dbg(dev, "Unable to set MTU to include for DSA overheads\n");
> -	}
> -	rtnl_unlock();
> -}
> -
>  static void dsa_master_reset_mtu(struct net_device *dev)
>  {
>  	int err;
> @@ -344,7 +330,14 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
>  {
>  	int ret;
>  
> -	dsa_master_set_mtu(dev,  cpu_dp);
> +	rtnl_lock();
> +	ret = dev_set_mtu(dev, ETH_DATA_LEN + cpu_dp->tag_ops->overhead);
> +	rtnl_unlock();
> +	if (ret) {
> +		netdev_err(dev, "error %d setting MTU to include DSA overhead\n",
> +			   ret);
> +		return ret;
> +	}

I suspect this will break devices. dsa_master_set_mtu() was not fatal
if it failed. I did this deliberately because i suspect there are some
MAC drivers which are unhappy to have the MTU changed, but will still
send and receive frames which are bigger than the MTU. 

So please keep setting the MTU of ETH_DATA_LEN +
cpu_dp->tag_ops->overhead or less as non-fatal. Jumbo frame sizes you
should however check the return code.

> @@ -1465,7 +1556,10 @@ int dsa_slave_create(struct dsa_port *port)
>  	slave_dev->priv_flags |= IFF_NO_QUEUE;
>  	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>  	slave_dev->min_mtu = 0;
> -	slave_dev->max_mtu = ETH_MAX_MTU;
> +	if (ds->ops->port_max_mtu)
> +		slave_dev->max_mtu = ds->ops->port_max_mtu(ds, port->index);
> +	else
> +		slave_dev->max_mtu = ETH_MAX_MTU;

Does this bring you anything. You have a lot more checks you perform
when performing the actual change. Seems better to keep them all
together.

	Andrew