Re: [PATCH] net: Fix inconsistent teardown and release of private netdev state.
flat view
From: David Miller <davem@davemloft.net>
Date: 2017-06-09 14:17:35
From: Johannes Berg <johannes@sipsolutions.net> Date: Fri, 09 Jun 2017 14:45:25 +0200
Under drivers/ in general, I count more than 400 calls to register_netdev[ice](), but only 39 that set the new needs_free_netdev. Some will overlap and have the same ops, but still, that's a rather small portion of them. The logic means those that don't set needs_free_netdev can't really use the new priv_destructor (previously destructor), I think? It seems to me that priv_destructor should imply needs_free_netdev (though not necessarily the other way around). I guess this must mean that that all others are dealing with the problem "manually", right? Perhaps needs_free_netdev isn't all that necessary then?
Yeah, the major two modes of operation are manual freeing of the netdev (usually at module unload time or similar) or via the destructor mechanisms.
quoted hunk ↗ jump to hunk
I think you introduced a bug though - isn't this needed?--- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c@@ -1816,6 +1816,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,ret = dev_alloc_name(ndev, ndev->name); if (ret < 0) { ieee80211_if_free(ndev); + free_netdev(ndev); return ret; } There was another caller of ieee80211_if_free() which you modified, and thus needs the free_netdev() that you removed from it.
I think you are right. The pattern is that when something fails after allocating the netdev, but before registering it, that code must free the netdev itself.
quoted hunk ↗ jump to hunk
Would an alternative have been to not use (priv_)destructor here, and just free all the data after unregister_netdevice(_many)? This sets the reg_state to NETREG_UNREGISTERING, so sysfs can no longer look at the stats afterwards, and it's been unlisted (unlist_netdevice) so can't be reached through any other means either. IOW, this would also work and fix the bug above along the way?diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 915d7e1b4545..23df973d5181 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c
...
quoted hunk ↗ jump to hunk
@@ -1932,6 +1931,7 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata) if (sdata->dev) { unregister_netdevice(sdata->dev); + ieee80211_if_free(sdata->dev); } else { cfg80211_unregister_wdev(&sdata->wdev); ieee80211_teardown_sdata(sdata);
Unless I misunderstood something, unregister_netdevice() here will invoke the ->priv_destructor() and thus now it will be invoked twice?