linux-kernel - Re: [PATCH net-next v5 02/11] net: ti: prueth: Adds ICSSM Ethernet driver

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: <440344110.1093115.1744978988608.JavaMail.zimbra@couthit.local>
Date: Fri, 18 Apr 2025 17:53:08 +0530 (IST)
From: Parvathi Pudi <parvathi@...thit.com>
To: horms <horms@...nel.org>
Cc: parvathi <parvathi@...thit.com>, danishanwar <danishanwar@...com>, 
	rogerq <rogerq@...nel.org>, andrew+netdev <andrew+netdev@...n.ch>, 
	davem <davem@...emloft.net>, edumazet <edumazet@...gle.com>, 
	kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>, 
	robh <robh@...nel.org>, krzk+dt <krzk+dt@...nel.org>, 
	conor+dt <conor+dt@...nel.org>, nm <nm@...com>, 
	ssantosh <ssantosh@...nel.org>, tony <tony@...mide.com>, 
	richardcochran <richardcochran@...il.com>, 
	glaroque <glaroque@...libre.com>, schnelle <schnelle@...ux.ibm.com>, 
	m-karicheri2 <m-karicheri2@...com>, s hauer <s.hauer@...gutronix.de>, 
	rdunlap <rdunlap@...radead.org>, diogo ivo <diogo.ivo@...mens.com>, 
	basharath <basharath@...thit.com>, 
	jacob e keller <jacob.e.keller@...el.com>, 
	m-malladi <m-malladi@...com>, 
	javier carrasco cruz <javier.carrasco.cruz@...il.com>, 
	afd <afd@...com>, s-anna <s-anna@...com>, 
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, 
	netdev <netdev@...r.kernel.org>, 
	devicetree <devicetree@...r.kernel.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	pratheesh <pratheesh@...com>, Prajith Jayarajan <prajith@...com>, 
	Vignesh Raghavendra <vigneshr@...com>, praneeth <praneeth@...com>, 
	srk <srk@...com>, rogerq <rogerq@...com>, 
	krishna <krishna@...thit.com>, pmohan <pmohan@...thit.com>, 
	mohan <mohan@...thit.com>
Subject: Re: [PATCH net-next v5 02/11] net: ti: prueth: Adds ICSSM Ethernet
 driver

Hi,

> On Mon, Apr 14, 2025 at 05:04:49PM +0530, Parvathi Pudi wrote:
>> From: Roger Quadros <rogerq@...com>
>> 
>> Updates Kernel configuration to enable PRUETH driver and its dependencies
>> along with makefile changes to add the new PRUETH driver.
>> 
>> Changes includes init and deinit of ICSSM PRU Ethernet driver including
>> net dev registration and firmware loading for DUAL-MAC mode running on
>> PRU-ICSS2 instance.
>> 
>> Changes also includes link handling, PRU booting, default firmware loading
>> and PRU stopping using existing remoteproc driver APIs.
>> 
>> Signed-off-by: Roger Quadros <rogerq@...com>
>> Signed-off-by: Andrew F. Davis <afd@...com>
>> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>
>> Signed-off-by: Parvathi Pudi <parvathi@...thit.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>> b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> 
> ...
> 
>> +static int icssm_prueth_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *eth0_node = NULL, *eth1_node = NULL;
>> +	struct device_node *eth_node, *eth_ports_node;
>> +	enum pruss_pru_id pruss_id0, pruss_id1;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np;
>> +	struct prueth *prueth;
>> +	int i, ret;
>> +
>> +	np = dev->of_node;
>> +	if (!np)
>> +		return -ENODEV; /* we don't support non DT */
>> +
>> +	prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL);
>> +	if (!prueth)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, prueth);
>> +	prueth->dev = dev;
>> +	prueth->fw_data = device_get_match_data(dev);
>> +
>> +	eth_ports_node = of_get_child_by_name(np, "ethernet-ports");
>> +	if (!eth_ports_node)
>> +		return -ENOENT;
>> +
>> +	for_each_child_of_node(eth_ports_node, eth_node) {
>> +		u32 reg;
>> +
>> +		if (strcmp(eth_node->name, "ethernet-port"))
>> +			continue;
>> +		ret = of_property_read_u32(eth_node, "reg", &reg);
>> +		if (ret < 0) {
>> +			dev_err(dev, "%pOF error reading port_id %d\n",
>> +				eth_node, ret);
>> +		}
>> +
>> +		of_node_get(eth_node);
> 
> Hi Roger, Parvathi, all,
> 
> I feel that I'm missing something obvious here.
> But I have some questions about the reference to eth_node
> taken on the line above.
> 
>> +
>> +		if (reg == 0) {
>> +			eth0_node = eth_node;
> 
> If, while iterating through the for loop above, we reach this point more
> than once, then will the reference to the previously node assigned to
> eth0_node be leaked?
> 

We will modify the condition as below to avoid leaks
if ((reg == 0) && (eth0_node == NULL))

>> +			if (!of_device_is_available(eth0_node)) {
>> +				of_node_put(eth0_node);
>> +				eth0_node = NULL;
>> +			}
>> +		} else if (reg == 1) {
>> +			eth1_node = eth_node;
> 
> Likewise here for eth1_node.
> 

We will modify this also as below
if ((reg == 1) && (eth1_node == NULL))

>> +			if (!of_device_is_available(eth1_node)) {
>> +				of_node_put(eth1_node);
>> +				eth1_node = NULL;
>> +			}
>> +		} else {
>> +			dev_err(dev, "port reg should be 0 or 1\n");
> 
> And, perhaps more to the point, is the reference to eth_node leaked if
> we reach this line?
> 

We will check and add of_node_put(eth_node) at the end of the for loop.

>> +		}
>> +	}
>> +
>> +	of_node_put(eth_ports_node);
>> +
>> +	/* At least one node must be present and available else we fail */
>> +	if (!eth0_node && !eth1_node) {
>> +		dev_err(dev, "neither port0 nor port1 node available\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (eth0_node == eth1_node) {
> 
> Given the if / else if condition in the for loop above,
> I'm not sure this can ever occur.
> 

We will remove this.


Thanks and Regards,
Parvathi.