netdev - Re: Possible bug into DSA2 code.

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: <fe3891e6-4a91-f345-f543-60e19b006be3@gmail.com>
Date:   Mon, 11 Feb 2019 09:28:37 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rodolfo Giometti <giometti@...eenne.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: Possible bug into DSA2 code.

On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
> On 09/02/2019 20:34, Andrew Lunn wrote:
>>> So we I see two possible solutions:
>>>
>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>> defined is an
>>> error, then it must be signaled to the calling code, or
>>
>> I don't think we can do that. mv88e6xxx optionally instantiates the
>> MDIO busses, depending on what is in device tree. If there is no mdio
>> property, we need the DSA core to create an MDIO bus.
> 
> OK, but using the following check to know if DSA did such allocation is
> not correct because DSA drivers can allocate it by their own:
> 
> static void dsa_switch_teardown(struct dsa_switch *ds)
> {
>         if (ds->slave_mii_bus && ds->ops->phy_read)
>                 mdiobus_unregister(ds->slave_mii_bus);
> 
> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?

If drivers allocate the slave_mii_bus, or use it as a pointer to their
bus, then they should not be providing a ds->ops->phy_read() callback
since we assume they would have mii_bus::read and mii_bus::write set to
their driver internal version.

> 
>> Looking at the driver, ds->slave_mii_bus is assigned in
>> mv88e6xxx_setup().
>>
>> We have talked about adding a teardown() to the ops structure. This
>> seems like another argument we should do it. The mv88e6xxx_teardown()
>> can set ds->slave_mii_bus back to NULL, undoing what it did in the
>> setup code.
> 
> This seems reasonable to me, but in this case you have to call
> teardown() operation before calling mdiobus_unregister() into
> dsa_switch_teardown() or we still have the problem...
> 
> Ciao,
> 
> Rodolfo
> 


-- 
Florian