netdev - Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add RXNFC 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190907203256.GA18741@lunn.ch>
Date:   Sat, 7 Sep 2019 22:32:56 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, f.fainelli@...il.com
Subject: Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add RXNFC support

> +static int mv88e6xxx_policy_insert(struct mv88e6xxx_chip *chip, int port,
> +				   struct ethtool_rx_flow_spec *fs)
> +{
> +	struct ethhdr *mac_entry = &fs->h_u.ether_spec;
> +	struct ethhdr *mac_mask = &fs->m_u.ether_spec;
> +	enum mv88e6xxx_policy_mapping mapping;
> +	enum mv88e6xxx_policy_action action;
> +	struct mv88e6xxx_policy *policy;
> +	u16 vid = 0;
> +	u8 *addr;
> +	int err;
> +	int id;
> +
> +	if (fs->location != RX_CLS_LOC_ANY)
> +		return -EINVAL;
> +
> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC)
> +		action = MV88E6XXX_POLICY_ACTION_DISCARD;
> +	else
> +		return -EOPNOTSUPP;
> +
> +	switch (fs->flow_type & ~FLOW_EXT) {
> +	case ETHER_FLOW:
> +		if (!is_zero_ether_addr(mac_mask->h_dest) &&
> +		    is_zero_ether_addr(mac_mask->h_source)) {
> +			mapping = MV88E6XXX_POLICY_MAPPING_DA;
> +			addr = mac_entry->h_dest;
> +		} else if (is_zero_ether_addr(mac_mask->h_dest) &&
> +		    !is_zero_ether_addr(mac_mask->h_source)) {
> +			mapping = MV88E6XXX_POLICY_MAPPING_SA;
> +			addr = mac_entry->h_source;
> +		} else {
> +			/* Cannot support DA and SA mapping in the same rule */
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if ((fs->flow_type & FLOW_EXT) && fs->m_ext.vlan_tci) {
> +		if (fs->m_ext.vlan_tci != 0xffff)
> +			return -EOPNOTSUPP;
> +		vid = be16_to_cpu(fs->h_ext.vlan_tci) & VLAN_VID_MASK;
> +	}
> +
> +	idr_for_each_entry(&chip->policies, policy, id) {
> +		if (policy->port == port && policy->mapping == mapping &&
> +		    policy->action == action && policy->vid == vid &&
> +		    ether_addr_equal(policy->addr, addr))
> +			return -EEXIST;
> +	}
> +
> +	policy = devm_kzalloc(chip->dev, sizeof(*policy), GFP_KERNEL);
> +	if (!policy)
> +		return -ENOMEM;

Hi Vivien

I think this might be the first time we have done dynamic memory
allocation in the mv88e6xxx driver. It might even be a first for a DSA
driver?

I'm not saying it is wrong, but maybe we should discuss it. 

I assume you are doing this because the ATU entry itself is not
sufficient?

How much memory is involved here, worst case? I assume one struct
mv88e6xxx_policy per ATU entry? Which you think is too much to
allocate as part of chip? I guess most users will never use this
feature, so for most users it would be wasted memory. So i do see the
point for dynamically allocating it.

Thanks
	Andrew