RE: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

[Wells Lu 呂芳騰]

Hi Pavel,

> Hi, Wells!
> 
> On 11/3/21 14:02, Wells Lu wrote:
> 
> [code snip]
> 
> > +		if (comm->dual_nic) {
> > +			struct net_device *net_dev2 = mac->next_netdev;
> > +
> > +			if (!netif_running(net_dev2)) {
> > +				mac_hw_stop(mac);
> > +
> > +				mac2 = netdev_priv(net_dev2);
> > +
> 
> (*)
> 
> > +				// unregister and free net device.
> > +				unregister_netdev(net_dev2);
> > +				free_netdev(net_dev2);
> > +				mac->next_netdev = NULL;
> > +				pr_info(" Unregistered and freed net device \"eth1\"!\n");
> > +
> > +				comm->dual_nic = 0;
> > +				mac_switch_mode(mac);
> > +				rx_mode_set(net_dev);
> > +				mac_hw_addr_del(mac2);
> > +
> 
> mac2 is net_dev2 private data (*), so it will become freed after
> free_netdev() call.
> 
> FWIW the latest `smatch` should warn about this type of bugs.

Yes, this is indeed a bug.
But the code paragraph has been removed thoroughly in [PATCH v2].


> > +				// If eth0 is up, turn on lan 0 and 1 when
> > +				// switching to daisy-chain mode.
> > +				if (comm->enable & 0x1)
> > +					comm->enable = 0x3;
> 
> [code snip]
> 
> > +static int l2sw_remove(struct platform_device *pdev) {
> > +	struct net_device *net_dev;
> > +	struct net_device *net_dev2;
> > +	struct l2sw_mac *mac;
> > +
> > +	net_dev = platform_get_drvdata(pdev);
> > +	if (!net_dev)
> > +		return 0;
> > +	mac = netdev_priv(net_dev);
> > +
> > +	// Unregister and free 2nd net device.
> > +	net_dev2 = mac->next_netdev;
> > +	if (net_dev2) {
> > +		unregister_netdev(net_dev2);
> > +		free_netdev(net_dev2);
> > +	}
> > +
> 
> Is it save here to free mac->next_netdev before unregistering "parent"
> netdev? I haven't checked the whole code, just asking :)

Yes, I think it is save.
netdev2 should be unregistered and freed before net_dev.
If net_dev is unregistered and freed in advance,
mac->next_netdev becomes danger because 'mac' has been freed.


> > +	sysfs_remove_group(&pdev->dev.kobj, &l2sw_attribute_group);
> > +
> > +	mac->comm->enable = 0;
> > +	soc0_stop(mac);
> > +
> > +	napi_disable(&mac->comm->rx_napi);
> > +	netif_napi_del(&mac->comm->rx_napi);
> > +	napi_disable(&mac->comm->tx_napi);
> > +	netif_napi_del(&mac->comm->tx_napi);
> > +
> > +	mdio_remove(net_dev);
> > +
> > +	// Unregister and free 1st net device.
> > +	unregister_netdev(net_dev);
> > +	free_netdev(net_dev);
> > +
> > +	clk_disable(mac->comm->clk);
> > +
> > +	// Free 'common' area.
> > +	kfree(mac->comm);
> 
> Same here with `mac`.

This is indeed a bug.
But the statement, Kfree(mac->comm);, has been removed in [PATCH v2].
In [PATCH v2], structure data 'mac->comm' is allocated by
devm_kzalloc(). No more need to free it here.


> > +	return 0;
> > +}
> 
> 
> I haven't read the whole thread, i am sorry if these questions were already discussed.
> 
> 
> 
> With regards,
> Pavel Skripkin


Thank you very much for your review!

Best regards,
Wells Lu