backport: handle change in netdevice destructor usage

Commit Message

Arend van Spriel June 14, 2017, 5:28 p.m. UTC

On 14-06-17 15:26, Johannes Berg wrote:
> On Wed, 2017-06-14 at 15:25 +0200, Arend van Spriel wrote:
>> On 13-06-17 22:59, Johannes Berg wrote:
>>> On Mon, 2017-06-12 at 10:06 +0100, Arend van Spriel wrote:
>>>> This patch deals with changes made in struct net_device by commit
>>>> cf124db566e6 ("net: Fix inconsistent teardown and release of
>>>> private
>>>> netdev state."). This only looks for instances that need
>>>> free_netdev() call, ie. struct net_device::needs_free_netdev ==
>>>> true.
>>>
>>> Come to think of it, isn't this missing the part where we now call
>>> priv_destructor when registering fails or something?
>>
>> Ah. I should have studied the patch better to see what behavioral
>> changes the commit imposed. 
> 
> Not sure, tbh. I just think there are issues there.
> 
>> So are you still considering the patch
>> despite the likely backport of commit cf124db566e6. I can do my
>> homework better and resubmit if needed.
> 
> Yeah I still think this backports patch makes sense, since I'm not sure
> I want to fix mac80211 that way (the bits in unregistering multiple
> netdevs are awkward) and brcmfmac would still need it.

So I changed the semantic patch as below, which fixes the error path
when register_netdevice() fails:

@@ -1884,6 +1889,7 @@ int ieee80211_if_add(struct ieee80211_lo

 		ret = register_netdevice(ndev);
 		if (ret) {
+			ieee80211_if_free(ndev);
 			free_netdev(ndev);
 			return ret;
 		}

Unfortunately, it also creates the following hunk:

@@ -1779,6 +1783,7 @@ int ieee80211_if_add(struct ieee80211_lo

 		ndev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
 		if (!ndev->tstats) {
+			ieee80211_if_free(ndev);
 			free_netdev(ndev);
 			return -ENOMEM;
 		}

All ieee80211_if_free() does is freeing the tstats and it deals with
null pointers. So for mac80211 this is ok but not generically. Not sure
how to tackle this in a semantic patch.

Regards,
Arend
---

--
To unsubscribe from this list: send the line "unsubscribe backports" in

Comments

Johannes Berg June 14, 2017, 8:22 p.m. UTC | #1

On Wed, 2017-06-14 at 19:28 +0200, Arend van Spriel wrote:
> 
> All ieee80211_if_free() does is freeing the tstats and it deals with
> null pointers. So for mac80211 this is ok but not generically. Not
> sure how to tackle this in a semantic patch.

I think the problem only applies to the register_netdev[ice]() error
path directly, no? That is, you should be able to restrict it to that
having just failed?

johannes
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Patch

diff --git a/patches/0079-netdev-destructor.cocci
b/patches/0079-netdev-destructor.cocci
index d8a439d..71b5525 100644
--- a/patches/0079-netdev-destructor.cocci
+++ b/patches/0079-netdev-destructor.cocci
@@ -14,6 +14,15 @@  C(...)
 @r2 depends on r1@
 struct net_device *NDEV;
 identifier r1.D;
+@@
+
+-      free_netdev(NDEV);
++      D(NDEV);
++      free_netdev(NDEV);
+
+@r3 depends on r2@
+struct net_device *NDEV;
+identifier r1.D;
 identifier r1.C;
 fresh identifier E2 = "__" ## D;
 @@