netdev - Re: [PATCH] phy: fix possible double lock calling link changed handler

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:   Tue, 23 Nov 2021 01:49:46 -0300
From:   Alessandro B Maurici <abmaurici@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] phy: fix possible double lock calling link changed
 handler

On Tue, 23 Nov 2021 05:18:14 +0100
Andrew Lunn <andrew@...n.ch> wrote:

> On Mon, Nov 22, 2021 at 11:55:48PM -0300, Alessandro B Maurici wrote:
> > From: Alessandro B Maurici <abmaurici@...il.com>
> > 
> > Releases the phy lock before calling phy_link_change to avoid any worker
> > thread lockup. Some network drivers(eg Microchip's LAN743x), make a call to
> > phy_ethtool_get_link_ksettings inside the link change handler  
> 
> I think we need to take a step back here and answer the question, why
> does it call phy_ethtool_get_link_ksettings in the link change
> handler. I'm not aware of any other MAC driver which does this.
> 
> 	 Andrew

I agree, the use in the lan743x seems related to the PTP, that driver seems
to be the only one using it, at least in the Linus tree. 
I think that driver could be patched as there are other ways to do it,
but my take on the problem itself is that the PHY device interface opens
a way to break the flow and this behavior does not seem to be documented,
so, instead of documenting a possible harmful interface while in the callback,
we should just get rid of the problem itself, and calling a callback without
any locks held seems to be a good alternative.
This is also a non critical performance path and the additional code
would not impact much, of course it makes the stuff less nice to look at.
The patch also has an additional check for the lock, since there is a 
function that is not calling the lock explicitly and has a warn if the lock
is not held at the start, so I put it there to be extra safe.

Alessandro