netdev - Re: [PATCH net-next 4/9] net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii

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:	Sun, 5 Jun 2016 15:38:59 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Andrew Lunn <andrew@...n.ch>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	vivien.didelot@...oirfairelinux.com, john@...ozen.org
Subject: Re: [PATCH net-next 4/9] net: dsa: Initialize ds->enabled_port_mask
 and ds->phys_mii_mask

Le 04/06/2016 13:29, Andrew Lunn a écrit :
>> @@ -517,6 +541,15 @@ static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds)
>>  			return -EINVAL;
>>  
>>  		ds->ports[reg].dn = port;
>> +
>> +		if (dsa_port_is_cpu(port))
>> +			ds->dst->cpu_port = reg;
>> +		else
>> +			/* Initialize enabled_port_mask now for drv->setup()
>> +			 * to have access to a correct value, just like what
>> +			 * net/dsa/dsa.c::dsa_switch_setup_one does.
>> +			 */
>> +			ds->enabled_port_mask |= 1 << reg;
> 
> Hi Florian
> 
> You need to be careful here. There can be multiple CPU ports, in
> different switches. We want dst->cpu_port to be deterministic,
> independent of the order switches are registered. Which is why i set
> it as part of dsa_cpu_parse(), which only happens when all the
> switches have registered, and we are parsing their device tree nodes
> in order. So we guarantee dst->cpu_port is the first CPU node.

Ah OK, I completely missed that part and just wanted to avoid walking
the ports children nodes more than twice.

We might be able to get away with just initializing
ds->enabled_port_mask here actually.

> 
> You now set dst->cpu_port via dsa_parse_ports_dn(), so it is now non
> deterministic, it depends on the probe order of the switches.
> 
> In the long run, i want to deprecate and then remove dst->cpu_port,
> but i'm not that far yet.
> 
> Please rethink this part of the patch, keeping in mind you have
> multiple switches, with multiple CPU and DSA ports, all connected in
> some crazy fashion.
-- 
Florian