From: Andrew Lunn <andrew@lunn•ch>
To: Alessandro B Maurici <abmaurici@gmail•com>
Cc: netdev@vger•kernel.org, Heiner Kallweit <hkallweit1@gmail•com>,
Russell King <linux@armlinux•org.uk>,
"David S. Miller" <davem@davemloft•net>
Subject: Re: [PATCH] phy: fix possible double lock calling link changed handler
Date: Tue, 23 Nov 2021 15:09:04 +0100 [thread overview]
Message-ID: <YZz2AJ+wqasknw2p@lunn.ch> (raw)
In-Reply-To: <20211123014946.1ec2d7ee@work>
On Tue, Nov 23, 2021 at 01:49:46AM -0300, Alessandro B Maurici wrote:
> On Tue, 23 Nov 2021 05:18:14 +0100
> Andrew Lunn <andrew@lunn•ch> wrote:
>
> > On Mon, Nov 22, 2021 at 11:55:48PM -0300, Alessandro B Maurici wrote:
> > > From: Alessandro B Maurici <abmaurici@gmail•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.
That is a really bad alternative. It is only because the lock is held
can the MAC driver actually trust anything passed to it. The callback
needs phydev->speed, phydev->duplex, etc, and they can change at any
time when the lock is not held. The values can be inconsistent with
each other, etc, unless the lock is held.
The callback has always had the lock held, so is safe. However,
recently a few bugs have been reported and fixed for functions like
phy_ethtool_get_link_ksettings() and phy_ethtool_set_link_ksettings()
where they have accessed phydev members without the lock and got
inconsistent values in race condition. These are hard race conditions
to reproduce, but a deadlock like this is very obvious, easy to fix. I
would also say that _ethtool_ in the function name is also a good hit
this is intended to be used for an ethtool callback.
Lets remove the inappropriate use of phy_ethtool_get_link_ksettings()
here.
Andrew
next prev parent reply other threads:[~2021-11-23 14:09 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 2:55 [PATCH] phy: fix possible double lock calling link changed handler Alessandro B Maurici
2021-11-23 4:18 ` Andrew Lunn
2021-11-23 4:49 ` Alessandro B Maurici
2021-11-23 8:21 ` Heiner Kallweit
2021-11-23 14:11 ` Andrew Lunn
2021-11-23 16:06 ` Alessandro B Maurici
2021-11-23 20:32 ` Heiner Kallweit
2021-11-23 22:31 ` Alessandro B Maurici
2021-11-23 14:09 ` Andrew Lunn [this message]
2021-11-23 14:14 ` Russell King (Oracle)
2021-11-23 15:58 ` Alessandro B Maurici
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YZz2AJ+wqasknw2p@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=abmaurici@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=hkallweit1@gmail$(echo .)com \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox