netdev - Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

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: <ZB3YGWTWLYyecgw7@shell.armlinux.org.uk>
Date:   Fri, 24 Mar 2023 17:04:25 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>, linux-acpi@...r.kernel.org,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software
 node for default settings

On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> Hi Russell,
> 
> On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > +							   int speed,
> > +							   int duplex)
> > +{
> > +	struct property_entry fixed_link_props[3] = { };
> > +
> > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > +	if (duplex == DUPLEX_FULL)
> > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > +
> > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > +						 "fixed-link");
> > +}
> > +
> > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > +							  int speed,
> > +							  int duplex)
> > +{
> > +	struct property_entry port_props[2] = {};
> > +	struct fwnode_handle *fixed_link_fwnode;
> > +	struct fwnode_handle *new_port_fwnode;
> > +
> > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > +	if (IS_ERR(new_port_fwnode))
> > +		return new_port_fwnode;
> > +
> > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > +							  speed, duplex);
> > +	if (IS_ERR(fixed_link_fwnode)) {
> > +		fwnode_remove_software_node(new_port_fwnode);
> > +		return fixed_link_fwnode;
> > +	}
> > +
> > +	return new_port_fwnode;
> > +}
> 
> That new fwnode_create_named_software_node() function looks like a
> conflict waiting to happen - if a driver adds a node to the root level
> (does not have to be root level), all the tests will pass because
> there is only a single device, but when a user later tries the driver
> with two devices, it fails, because the node already exist. But you
> don't need that function at all.

I think you're totally failing to explain how this can fail.

Let me reiterate what thestructure of the swnodes here is:

	root
	`- node%d (%d allocated by root IDA)
	   +- phy-mode property
	   `- fixed-link
	      +- speed property
	      `- optional full-duplex property

If we have two different devices creating these nodes, then at the
root level, they will end up having different root names. The
"fixed-link" is a child of this node.

swnode already allows multiple identical names at the sub-node
level - each node ends up with its own IDA to allocate the generic
"node%d" names from. So as soon as we have multiple nodes, they
end up as this:

	root
	+- node0
	|  `- node 0
	+- node1
	|  `- node 0
	+- node2
	|  `- node 0
	etc

So, if we end up with two devices creating these at the same time,
we end up with:

	root
	+- nodeA (A allocated by root IDA)
	|  +- phy-mode property
	|  `- fixed-link
	|     +- speed property
	|     `- optional full-duplex property
	`- nodeB (B allocated by root IDA, different from above)
	   +- phy-mode property
	   `- fixed-link
	      +- speed property
	      `- optional full-duplex property

Since the kobject is parented to the parent's kobject, what we
end up with in sysfs is:

	.../nodeA/fixed-link/speed
	.../nodeB/fixed-link/speed

Thus, the "fixed-link" ndoes can _not_ conflict.

Please explain in detail where you think the conflict is, because
so far no one has been able to counter my assertions that this is
_safe_ with a proper full technical description of the problem.
All I get is hand-wavey "this conflicts".

Honestly, I'm getting sick of poor quality reviews... the next
poor review that claims there's a conflict here without properly
explain it will be told where to go.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!