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:   Mon, 11 Feb 2019 11:13:59 -0800
From:   Florian Fainelli <>
To:     Rodolfo Giometti <>,
        Andrew Lunn <>
Cc:     Vivien Didelot <>,
        "David S. Miller" <>,
Subject: Re: Possible bug into DSA2 code.

On 2/11/19 10:01 AM, Florian Fainelli wrote:
> 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.

Does that work:

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index a1917025e155..54cf6a5c865d 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -368,6 +368,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
        if (err)
                return err;

+       if (ds->slave_mii_bus && (ds->ops->phy_read || ds->ops->phy_write))
+               return -EINVAL;
        if (!ds->slave_mii_bus && ds->ops->phy_read) {
                ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
                if (!ds->slave_mii_bus)
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index cb42939db776..0796c6213be6 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -176,6 +176,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
        if (ret)
                return ret;

+       if (ds->slave_mii_bus && (ops->phy_read || ops->phy_write))
+               return -EINVAL;
        if (!ds->slave_mii_bus && ops->phy_read) {
                ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
                if (!ds->slave_mii_bus)



Powered by blists - more mailing lists