netdev - Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

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]

Date:   Sat, 2 Jul 2022 14:12:20 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "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 net-next v3 5/7] net: lan966x: Add lag support for
 lan966x.

On Fri, Jul 01, 2022 at 10:52:25PM +0200, Horatiu Vultur wrote:
> Add link aggregation hardware offload support for lan966x
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> ---

Superficially the code looks OK (maybe if you could only remove the
trailing "." from the commit message). A comment below.

> +int lan966x_lag_port_prechangeupper(struct net_device *dev,
> +				    struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	struct netdev_lag_upper_info *lui;
> +	struct netlink_ext_ack *extack;
> +
> +	extack = netdev_notifier_info_to_extack(&info->info);
> +	lui = info->upper_info;
> +	if (!lui)
> +		return NOTIFY_DONE;
> +
> +	if (lui->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported Tx type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	switch (lui->hash_type) {
> +	case NETDEV_LAG_HASH_L2:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L34:
> +		lan_wr(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L23:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);

ANA_AGGR_CFG is global to the switch, yet it constantly changes with the
hash_type of each new offloaded LAG. So a LAG that hashes on L3/L4 will
break the hash mode of an existing L2 LAG. I think you should implement
a system that disallows changes to ANA_AGGR_CFG as long as there are
users of a certain mode.

> +		break;
> +	default:
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported hash type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	return NOTIFY_OK;
> +}