netdev - Re: [PATCH net-next v2 8/9] net: ethernet: ti: am65-cpsw: add network flow classification support

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] [day] [month] [year] [list]

Message-ID: <58dcc38b-8150-4272-b214-b046163292a6@kernel.org>
Date: Tue, 13 May 2025 14:41:57 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>, Siddharth Vadapalli
 <s-vadapalli@...com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>,
 danishanwar@...com
Cc: srk@...com, linux-omap@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 8/9] net: ethernet: ti: am65-cpsw: add network
 flow classification support



On 08/05/2025 17:00, Paolo Abeni wrote:
> On 5/5/25 6:26 PM, Roger Quadros wrote:
> [...]
>> +/* validate the rxnfc rule and convert it to policer config */
>> +static int am65_cpsw_rxnfc_validate(struct am65_cpsw_port *port,
>> +				    struct ethtool_rxnfc *rxnfc,
>> +				    struct cpsw_ale_policer_cfg *cfg)
>> +{
>> +	struct ethtool_rx_flow_spec *fs = &rxnfc->fs;
>> +	int flow_type = AM65_CPSW_FLOW_TYPE(fs->flow_type);
>> +	struct ethhdr *eth_mask;
> 
> (Minor nit only mentioned because of more relevant comments on previous
> patch) Please respect the reverse christmas tree order above.

Yes, will fix.
> 
>> +
>> +	memset(cfg, 0, sizeof(*cfg));
>> +
>> +	if (flow_type & FLOW_RSS)
>> +		return -EINVAL;
>> +
>> +	if (fs->location == RX_CLS_LOC_ANY ||
>> +	    fs->location >= port->rxnfc_max)
>> +		return -EINVAL;
>> +
>> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC)
>> +		cfg->drop = true;
>> +	else if (fs->ring_cookie > AM65_CPSW_MAX_QUEUES)
>> +		return -EINVAL;
>> +
>> +	cfg->port_id = port->port_id;
>> +	cfg->thread_id = fs->ring_cookie;
>> +
>> +	switch (flow_type) {
>> +	case ETHER_FLOW:
>> +		eth_mask = &fs->m_u.ether_spec;
>> +
>> +		/* etherType matching is supported by h/w but not yet here */
>> +		if (eth_mask->h_proto)
>> +			return -EINVAL;
>> +
>> +		/* Only support source matching addresses by full mask */
>> +		if (is_broadcast_ether_addr(eth_mask->h_source)) {
>> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_MACSRC;
>> +			ether_addr_copy(cfg->src_addr,
>> +					fs->h_u.ether_spec.h_source);
>> +		}
>> +
>> +		/* Only support destination matching addresses by full mask */
>> +		if (is_broadcast_ether_addr(eth_mask->h_dest)) {
>> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_MACDST;
>> +			ether_addr_copy(cfg->dst_addr,
>> +					fs->h_u.ether_spec.h_dest);
>> +		}
>> +
>> +		if ((fs->flow_type & FLOW_EXT) && fs->m_ext.vlan_tci) {
>> +			/* Don't yet support vlan ethertype */
>> +			if (fs->m_ext.vlan_etype)
>> +				return -EINVAL;
>> +
>> +			if (fs->m_ext.vlan_tci != VLAN_TCI_FULL_MASK)
>> +				return -EINVAL;
>> +
>> +			cfg->vid = FIELD_GET(VLAN_VID_MASK,
>> +					     ntohs(fs->h_ext.vlan_tci));
>> +			cfg->vlan_prio = FIELD_GET(VLAN_PRIO_MASK,
>> +						   ntohs(fs->h_ext.vlan_tci));
>> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_OVLAN;
>> +		}
>> +
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int am65_cpsw_policer_find_match(struct am65_cpsw_port *port,
>> +					struct cpsw_ale_policer_cfg *cfg)
>> +{
>> +	struct am65_cpsw_rxnfc_rule *rule;
>> +	int loc = -EINVAL;
>> +
>> +	mutex_lock(&port->rxnfc_lock);
>> +	list_for_each_entry(rule, &port->rxnfc_rules, list) {
>> +		if (!memcmp(&rule->cfg, cfg, sizeof(*cfg))) {
>> +			loc = rule->location;
>> +			break;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&port->rxnfc_lock);
>> +
>> +	return loc;
>> +}
>> +
>> +static int am65_cpsw_rxnfc_add_rule(struct am65_cpsw_port *port,
>> +				    struct ethtool_rxnfc *rxnfc)
>> +{
>> +	struct ethtool_rx_flow_spec *fs = &rxnfc->fs;
>> +	struct am65_cpsw_rxnfc_rule *rule;
>> +	struct cpsw_ale_policer_cfg cfg;
>> +	int loc, ret;
>> +
>> +	if (am65_cpsw_rxnfc_validate(port, rxnfc, &cfg))
>> +		return -EINVAL;
>> +
>> +	/* need to check if similar rule is already present at another location,
>> +	 * if yes error out
>> +	 */
>> +	loc = am65_cpsw_policer_find_match(port, &cfg);
>> +	if (loc >= 0 && loc != fs->location) {
>> +		netdev_info(port->ndev,
>> +			    "rule already exists in location %d. not adding\n",
>> +			    loc);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* delete exisiting rule */
>> +	if (loc >= 0) {
>> +		mutex_lock(&port->rxnfc_lock);
> 
> The rxnfc_lock mutex is released and re-aquired after the previous
> lookup. Con some other thread delete the matching rule in-between and
> add another one at a different location?

Good point. I will remove mutex acquiring from am65_cpsw_policer_find_match() and
add acquire it instead in the beginning of am65_cpsw_rxnfc_add_rule()

> 
> /P
> 

-- 
cheers,
-roger