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: <2651a882-c5ab-ba3b-ef4c-731dca90147d@gmail.com>
Date:   Mon, 11 Feb 2019 10:01:22 -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/11/19 9:51 AM, Rodolfo Giometti wrote:
> On 11/02/2019 18:28, Florian Fainelli wrote:
>> 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.
> 
> I see, so having ds->slave_mii_bus and ds->ops->phy_read both not NULL
> into dsa_switch_setup() is a potential bug, I suppose... If so why not
> adding a BUG_ON() call to signal it instead of doing nothing? :-o

If you have both non NULL, then your driver did allocate
ds->slave_mii_bus on its own, and also assigned a valid
ds->ops->phy_read() then things will work, except that
ds->ops->phy_read() will not be used. And yes, that is going to be
blowing away when the whole DSA tree gets teardowned.

If you want to add a check for that condition, that would be a good
thing, just not a BUG_ON(), propagate an error back to the caller and
abort the tree/switch probing.
-- 
Florian