All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: "Florian Fainelli" <f.fainelli@gmail.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Vladimir Oltean" <vladimir.oltean@nxp.com>,
	netdev@vger.kernel.org,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	linux-kernel@vger.kernel.org,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Alvin Šipraga" <alsi@bang-olufsen.dk>,
	"ACPI Devel Maling List" <linux-acpi@vger.kernel.org>,
	kernel-team <kernel-team@android.com>,
	"Len Brown" <lenb@kernel.org>
Subject: Re: [RFC PATCH net-next 1/3] net: phy: don't bind genphy in phy_attach_direct if the specific driver defers probe
Date: Fri, 3 Sep 2021 01:45:06 +0300	[thread overview]
Message-ID: <20210902224506.5h7bnybjbljs5uxz@skbuf> (raw)
In-Reply-To: <20210902222439.GQ22278@shell.armlinux.org.uk>

On Thu, Sep 02, 2021 at 11:24:39PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 03, 2021 at 12:39:49AM +0300, Vladimir Oltean wrote:
> > On Thu, Sep 02, 2021 at 10:33:03PM +0100, Russell King (Oracle) wrote:
> > > That's probably an unreliable indicator. DPAA2 has weirdness in the
> > > way it can dynamically create and destroy network interfaces, which
> > > does lead to problems with the rtnl lock. I've been carrying a patch
> > > from NXP for this for almost two years now, which NXP still haven't
> > > submitted:
> > >
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=cex7&id=a600f2ee50223e9bcdcf86b65b4c427c0fd425a4
> > >
> > > ... and I've no idea why that patch never made mainline. I need it
> > > to avoid the stated deadlock on SolidRun Honeycomb platforms when
> > > creating additional network interfaces for the SFP cages in userspace.
> >
> > Ah, nice, I've copied that broken logic for the dpaa2-switch too:
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d52ef12f7d6c016f3b249db95af33f725e3dd065
> >
> > So why don't you send the patch? I can send it too if you want to, one
> > for the switch and one for the DPNI driver.
>
> Sorry, I mis-stated. NXP did submit that exact patch, but it's actually
> incorrect for the reason I stated when it was sent:
>
> https://patchwork.ozlabs.org/project/netdev/patch/1574363727-5437-2-git-send-email-ioana.ciornei@nxp.com/

So why are you carrying it then?

> I did miss the rtnl_lock() around phylink_disconnect_phy() in the
> description of the race, which goes someway towards hiding it, but
> there is still a race between phylink_destroy() and another thread
> calling dpaa2_eth_get_link_ksettings(), and priv->mac being freed:
>
> static int
> dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
>                              struct ethtool_link_ksettings *link_settings)
> {
>         struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
>
>         if (dpaa2_eth_is_type_phy(priv))
>                 return phylink_ethtool_ksettings_get(priv->mac->phylink,
>                                                      link_settings);
>
> which dereferences priv->mac and priv->mac->phylink, vs:
>
> static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg)
> {
> ...
>         if (status & DPNI_IRQ_EVENT_ENDPOINT_CHANGED) {
>                 dpaa2_eth_set_mac_addr(netdev_priv(net_dev));
>                 dpaa2_eth_update_tx_fqids(priv);
>
>                 if (dpaa2_eth_has_mac(priv))
>                         dpaa2_eth_disconnect_mac(priv);
>                 else
>                         dpaa2_eth_connect_mac(priv);
>         }
>
> static void dpaa2_eth_disconnect_mac(struct dpaa2_eth_priv *priv)
> {
>         if (dpaa2_eth_is_type_phy(priv))
>                 dpaa2_mac_disconnect(priv->mac);
>
>         if (!dpaa2_eth_has_mac(priv))
>                 return;
>
>         dpaa2_mac_close(priv->mac);
>         kfree(priv->mac);		<== potential use after free bug by
>         priv->mac = NULL;		<== dpaa2_eth_get_link_ksettings()
> }

Okay, so this needs to stay under the rtnetlink mutex, to serialize with
dpaa2_eth_get_link_ksettings which is already under the rtnetlink mutex.
So the way in which rtnl_lock is taken right now is actually fine in a way.

>
> void dpaa2_mac_disconnect(struct dpaa2_mac *mac)
> {
>         if (!mac->phylink)
>                 return;
>
>         phylink_disconnect_phy(mac->phylink);
>         phylink_destroy(mac->phylink);	<== another use-after-free bug via
> 					    dpaa2_eth_get_link_ksettings()
>         dpaa2_pcs_destroy(mac);
> }
>
> Note that phylink_destroy() is documented as:
>
>  * Note: the rtnl lock must not be held when calling this function.
>
> because it calls sfp_bus_del_upstream(), which will take the rtnl lock
> itself. An alternative solution would be to remove the rtnl locking
> from sfp_bus_del_upstream(), but then force _everyone_ to take the
> rtnl lock before calling phylink_destroy() - meaning a larger block of
> code ends up executing under the lock than is really necessary.

So phylink_destroy has exactly 20 call sites, it is not that bad?

And as for "larger block than necessary" - doesn't the dpaa2 prolonged
usage count as necessary?

> However, as I stated in my review of the patch "As I've already stated,
> the phylink is not designed to be created and destroyed on a published
> network device." That still remains true today, and it seems that the
> issue has never been fixed in DPAA2 despite having been pointed out.

So what would you do, exactly, to "fix" the issue that a DPNI can
connect and disconnect at runtime from a DPMAC?

Also, "X is not designed to Y" doesn't really say much, given a bit of
will power. Linux was not designed to run on non-i386 either.

Any other issues besides needing to take rtnl_mutex top-level when
calling phylink_destroy? Since phylink_disconnect_phy needs it anyway,
and phylink_destroy ends up calling sfp_bus_del_upstream which takes the
same mutex again, and drivers that connect/disconnect at probe/remove
time end up calling both in a row, I don't think there is much of an
issue to speak of, or that the rework would be that difficult.

  reply	other threads:[~2021-09-02 22:45 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01 22:50 [RFC PATCH net-next 0/3] Make the PHY library stop being so greedy when binding the generic PHY driver Vladimir Oltean
2021-09-01 22:50 ` [RFC PATCH net-next 1/3] net: phy: don't bind genphy in phy_attach_direct if the specific driver defers probe Vladimir Oltean
2021-09-02  5:43   ` Greg Kroah-Hartman
2021-09-02 10:11     ` Vladimir Oltean
2021-09-02 10:37       ` Greg Kroah-Hartman
2021-09-02 11:17         ` Vladimir Oltean
2021-09-02 14:37     ` Rafael J. Wysocki
2021-09-02 18:50   ` Russell King (Oracle)
2021-09-02 19:23     ` Vladimir Oltean
2021-09-02 19:51     ` Andrew Lunn
2021-09-02 20:33       ` Florian Fainelli
2021-09-02 21:33         ` Russell King (Oracle)
2021-09-02 21:39           ` Vladimir Oltean
2021-09-02 22:24             ` Russell King (Oracle)
2021-09-02 22:45               ` Vladimir Oltean [this message]
2021-09-02 23:02                 ` Andrew Lunn
2021-09-02 23:26                   ` Vladimir Oltean
2021-09-03  0:04                     ` Russell King (Oracle)
2021-09-03 20:48                       ` Vladimir Oltean
2021-09-03 22:06                         ` Russell King (Oracle)
2021-09-04 21:59                           ` Vladimir Oltean
2021-09-04 23:25                             ` Russell King (Oracle)
2021-09-05  0:41                               ` Vladimir Oltean
2021-09-03  9:27               ` Ioana Ciornei
2021-09-01 22:50 ` [RFC PATCH net-next 2/3] net: dsa: destroy the phylink instance on any error in dsa_slave_phy_setup Vladimir Oltean
2021-09-02 12:25   ` Russell King (Oracle)
2021-09-02 23:21   ` Florian Fainelli
2021-09-01 22:50 ` [RFC PATCH net-next 3/3] net: dsa: allow the phy_connect() call to return -EPROBE_DEFER Vladimir Oltean
2021-09-02 12:19 ` [RFC PATCH net-next 0/3] Make the PHY library stop being so greedy when binding the generic PHY driver Russell King (Oracle)
2021-09-02 12:35   ` Vladimir Oltean
2021-09-02 12:59     ` Vladimir Oltean
2021-09-02 13:26     ` Russell King (Oracle)
2021-09-02 15:23       ` Vladimir Oltean
2021-09-02 16:31         ` Russell King (Oracle)
2021-09-02 17:10           ` Vladimir Oltean
2021-09-02 17:50             ` Russell King (Oracle)
2021-09-02 19:05               ` Vladimir Oltean
2021-09-02 20:03                 ` Russell King (Oracle)
2021-09-02 20:21                   ` Vladimir Oltean
2021-09-02 20:29                     ` Russell King (Oracle)
2021-09-03 16:22                       ` Vladimir Oltean
2021-09-03 17:21                         ` Andrew Lunn
2021-09-03 18:58                           ` Russell King (Oracle)
2021-09-03 19:56                             ` Andrew Lunn
2021-09-03 20:08                               ` Russell King (Oracle)
2021-09-03 18:54                         ` Russell King (Oracle)
2021-09-03 20:11                           ` Vladimir Oltean
2021-09-02 20:07     ` Andrew Lunn
2021-09-02 20:32       ` Vladimir Oltean
2021-09-02 21:39         ` Russell King (Oracle)
2021-09-02 22:05 ` Vladimir Oltean
2021-09-02 23:29   ` Saravana Kannan

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=20210902224506.5h7bnybjbljs5uxz@skbuf \
    --to=olteanv@gmail.com \
    --cc=alsi@bang-olufsen.dk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hkallweit1@gmail.com \
    --cc=kernel-team@android.com \
    --cc=kuba@kernel.org \
    --cc=lenb@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=vivien.didelot@gmail.com \
    --cc=vladimir.oltean@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.