| Message ID | 20201205191744.7847-12-rasmus.villemoes@prevas.dk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ethernet: ucc_geth: assorted fixes and simplifications | expand |
On Sat, 5 Dec 2020 20:17:34 +0100 Rasmus Villemoes wrote: > - unregister_netdev(dev); > - free_netdev(dev); > ucc_geth_memclean(ugeth); > if (of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > of_node_put(ugeth->ug_info->tbi_node); > of_node_put(ugeth->ug_info->phy_node); > + unregister_netdev(dev); > + free_netdev(dev); Are you sure you want to move the unregister_netdev() as well as the free?
On 05/12/2020 21.48, Jakub Kicinski wrote: > On Sat, 5 Dec 2020 20:17:34 +0100 Rasmus Villemoes wrote: >> - unregister_netdev(dev); >> - free_netdev(dev); >> ucc_geth_memclean(ugeth); >> if (of_phy_is_fixed_link(np)) >> of_phy_deregister_fixed_link(np); >> of_node_put(ugeth->ug_info->tbi_node); >> of_node_put(ugeth->ug_info->phy_node); >> + unregister_netdev(dev); >> + free_netdev(dev); > > Are you sure you want to move the unregister_netdev() as well as the > free? > Hm, dunno, I don't think it's needed per se, but it also shouldn't hurt from what I can tell. It seems more natural that they go together, but if you prefer a minimal patch that's of course also possible. I only noticed because I needed to add a free of the ug_info in a later patch. Rasmus
On Sat, 5 Dec 2020 22:04:28 +0100 Rasmus Villemoes wrote: > On 05/12/2020 21.48, Jakub Kicinski wrote: > > On Sat, 5 Dec 2020 20:17:34 +0100 Rasmus Villemoes wrote: > >> - unregister_netdev(dev); > >> - free_netdev(dev); > >> ucc_geth_memclean(ugeth); > >> if (of_phy_is_fixed_link(np)) > >> of_phy_deregister_fixed_link(np); > >> of_node_put(ugeth->ug_info->tbi_node); > >> of_node_put(ugeth->ug_info->phy_node); > >> + unregister_netdev(dev); > >> + free_netdev(dev); > > > > Are you sure you want to move the unregister_netdev() as well as the > > free? > > Hm, dunno, I don't think it's needed per se, but it also shouldn't hurt > from what I can tell. It seems more natural that they go together, but > if you prefer a minimal patch that's of course also possible. I was concerned about the fact that we free things and release references while the device may still be up (given that it's unregister_netdev() that will take it down). > I only noticed because I needed to add a free of the ug_info in a later > patch.
On 05/12/2020 22.19, Jakub Kicinski wrote: > On Sat, 5 Dec 2020 22:04:28 +0100 Rasmus Villemoes wrote: >> On 05/12/2020 21.48, Jakub Kicinski wrote: >>> On Sat, 5 Dec 2020 20:17:34 +0100 Rasmus Villemoes wrote: >>>> - unregister_netdev(dev); >>>> - free_netdev(dev); >>>> ucc_geth_memclean(ugeth); >>>> if (of_phy_is_fixed_link(np)) >>>> of_phy_deregister_fixed_link(np); >>>> of_node_put(ugeth->ug_info->tbi_node); >>>> of_node_put(ugeth->ug_info->phy_node); >>>> + unregister_netdev(dev); >>>> + free_netdev(dev); >>> >>> Are you sure you want to move the unregister_netdev() as well as the >>> free? >> >> Hm, dunno, I don't think it's needed per se, but it also shouldn't hurt >> from what I can tell. It seems more natural that they go together, but >> if you prefer a minimal patch that's of course also possible. > > I was concerned about the fact that we free things and release > references while the device may still be up (given that it's > unregister_netdev() that will take it down). I guess you're right. I'll fix it locally (and pull the patch earlier) and wait a few days with sending an updated version to give Li Yang some time to say if he wants to handle the series or not. Thanks, Rasmus
> I only noticed because I needed to add a free of the ug_info in a later > patch. Where, ironically, I add a use-after-free bug by freeing ug_info before the ucc_geth_memclean() call. :facepalm:
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index b132fcfc7c17..ba911d05d36d 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -3895,13 +3895,13 @@ static int ucc_geth_remove(struct platform_device* ofdev) struct ucc_geth_private *ugeth = netdev_priv(dev); struct device_node *np = ofdev->dev.of_node; - unregister_netdev(dev); - free_netdev(dev); ucc_geth_memclean(ugeth); if (of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); of_node_put(ugeth->ug_info->tbi_node); of_node_put(ugeth->ug_info->phy_node); + unregister_netdev(dev); + free_netdev(dev); return 0; }
ugeth is the netdiv_priv() part of the netdevice. Accessing the memory pointed to by ugeth (such as done by ucc_geth_memclean() and the two of_node_puts) after free_netdev() is thus use-after-free. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/ethernet/freescale/ucc_geth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)