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
