linux-kernel - Re: [PATCH net-next v6 02/12] net: pse-pd: Add support for reporting events

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: <20250320144320.53360553@kmaincent-XPS-13-7390>
Date: Thu, 20 Mar 2025 14:43:20 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>, Donald Hunter
 <donald.hunter@...il.com>, Rob Herring <robh@...nel.org>, Andrew Lunn
 <andrew+netdev@...n.ch>, Simon Horman <horms@...nel.org>, Heiner Kallweit
 <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Liam
 Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Thomas
 Petazzoni <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-doc@...r.kernel.org, Kyle Swenson <kyle.swenson@....tech>, Dent
 Project <dentproject@...uxfoundation.org>, kernel@...gutronix.de, Maxime
 Chevallier <maxime.chevallier@...tlin.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 02/12] net: pse-pd: Add support for
 reporting events

Hello Oleksij,

On Mon, 17 Mar 2025 10:28:58 +0100
Oleksij Rempel <o.rempel@...gutronix.de> wrote:

> Hi Köry,
> 
> sorry for late review.

No worry, thank you for the review! :)

> On Tue, Mar 04, 2025 at 11:18:51AM +0100, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@...tlin.com>  
> ...
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index aea0f03575689..9b41d4697a405 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -18,7 +18,8 @@ MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
> >  
> >  static struct pse_control *
> > -fwnode_find_pse_control(struct fwnode_handle *fwnode)
> > +fwnode_find_pse_control(struct fwnode_handle *fwnode,
> > +			struct phy_device *phydev)
> >  {
> >  	struct pse_control *psec;
> >  	struct device_node *np;
> > @@ -30,7 +31,7 @@ fwnode_find_pse_control(struct fwnode_handle *fwnode)
> >  	if (!np)
> >  		return NULL;
> >  
> > -	psec = of_pse_control_get(np);
> > +	psec = of_pse_control_get(np, phydev);
> >  	if (PTR_ERR(psec) == -ENOENT)
> >  		return NULL;
> >  
> > @@ -128,15 +129,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> >  	u32 phy_id;
> >  	int rc;
> >  
> > -	psec = fwnode_find_pse_control(child);
> > -	if (IS_ERR(psec))
> > -		return PTR_ERR(psec);
> > -
> >  	mii_ts = fwnode_find_mii_timestamper(child);
> > -	if (IS_ERR(mii_ts)) {
> > -		rc = PTR_ERR(mii_ts);
> > -		goto clean_pse;
> > -	}
> > +	if (IS_ERR(mii_ts))
> > +		return PTR_ERR(mii_ts);
> >  
> >  	is_c45 = fwnode_device_is_compatible(child,
> > "ethernet-phy-ieee802.3-c45"); if (is_c45 || fwnode_get_phy_id(child,
> > &phy_id)) @@ -169,6 +164,12 @@ int fwnode_mdiobus_register_phy(struct
> > mii_bus *bus, goto clean_phy;
> >  	}
> >  
> > +	psec = fwnode_find_pse_control(child, phy);
> > +	if (IS_ERR(psec)) {
> > +		rc = PTR_ERR(psec);
> > +		goto unregister_phy;
> > +	}  
> 
> Hm... we are starting to dereference the phydev pointer to a different
> framework without managing the ref-counting.
> 
> We will need to have some form of get_device(&phydev->mdio.dev).
> Normally it is done by phy_attach_direct().
> 
> And the counterpart: put_device() or phy_device_free()

The thing is that the pse_control is already related to the PHY. It is created
(pse_control_get_internal) when a PHY is
registered (fwnode_mdiobus_register_phy), and removed when the PHY is removed
(phy_device_remove).

If we add a get_device it will increase the refcount of the PHY device but we
won't be able to decrease the refcount as it will never enter the remove
callback due to that refcount.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com