netdev - Re: [PATCH rfc v0 9/9] net: dsa: qca8k: Move inband mutex into DSA core

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: <YyngzF8KyRAjoIn6@lunn.ch>
Date:   Tue, 20 Sep 2022 17:48:28 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     mattias.forsblad@...il.com, netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH rfc v0 9/9] net: dsa: qca8k: Move inband mutex into DSA
 core

On Tue, Sep 20, 2022 at 05:19:07AM +0200, Christian Marangi wrote:
> On Tue, Sep 20, 2022 at 12:18:53AM +0200, Andrew Lunn wrote:
> > The mutex serves two purposes:
> > 
> > It serialises operations on the switch, so that only one
> > request/response can be happening at once.
> > 
> > It protects priv->mgmt_master, which itself has two purposes.  If the
> > hardware is wrongly wires, the wrong switch port is connected to the
> > cpu, inband cannot be used. In this case it has the value
> > NULL. Additionally, if the master is down, it is set to
> > NULL. Otherwise it points to the netdev used to send frames to the
> > switch.
> > 
> > The protection of priv->mgmt_master is not required. It is a single
> > pointer, which will be updated atomically. It is not expected that the
> > interface disappears, it only goes down. Hence mgmt_master will always
> > be valid, or NULL.
> > 
> > Move the check for the master device being NULL into the core.  Also,
> > move the mutex for serialisation into the core.
> > 
> > The MIB operations don't follow request/response semantics, so its
> > mutex is left untouched.
> > 
> > Signed-off-by: Andrew Lunn <andrew@...n.ch>
> 
> BTW this patch makes the router crash with a kernel panic. Rest of the
> patchset works correctly and seems to be no regression. (I had to fix
> the clear_skb return value)

Thanks for testing.

As Vladimir pointed out, there is a mutex used in the wrong context. I
will fix that and the other issues pointed out, and see if i can spot
what i did wrong here. If not, we will have to decode the opps.

     Andrew