[PATCH] backport: handle change in netdevice destructor usage

[PATCH] backport: handle change in netdevice destructor usage

[Arend van Spriel]

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.

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 backport/backport-include/linux/netdevice.h | 12 +++++++++++
 patches/0079-netdev-destructor.cocci        | 33 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 patches/0079-netdev-destructor.cocci

diff --git a/backport/backport-include/linux/netdevice.h b/backport/backport-include/linux/netdevice.h
index 06230b5..4e30383 100644
--- a/backport/backport-include/linux/netdevice.h
+++ b/backport/backport-include/linux/netdevice.h
@@ -320,4 +320,16 @@ static inline void netif_trans_update(struct net_device *dev)
 }
 #endif
 
+static inline
+void netdev_set_priv_destructor(struct net_device *dev,
+				void (*destructor)(struct net_device *dev))
+{
+#if LINUX_VERSION_IS_LESS(4,12,0)
+	dev->destructor = destructor;
+#else
+	dev->needs_free_netdev = true;
+	dev->priv_destructor = destructor;
+#endif
+}
+
 #endif /* __BACKPORT_NETDEVICE_H */
diff --git a/patches/0079-netdev-destructor.cocci b/patches/0079-netdev-destructor.cocci
new file mode 100644
index 0000000..d8a439d
--- /dev/null
+++ b/patches/0079-netdev-destructor.cocci
@@ -0,0 +1,33 @@
+@r1@
+struct net_device *ndev;
+identifier D, C;
+@@
+C(...)
+{
+	<...
+-	ndev->needs_free_netdev = true;
+-	ndev->priv_destructor = D;
++	netdev_set_priv_destructor(ndev, D);
+	...>
+}
+
+@r2 depends on r1@
+struct net_device *NDEV;
+identifier r1.D;
+identifier r1.C;
+fresh identifier E2 = "__" ## D;
+@@
+
++static void E2(struct net_device *dev)
++{
++	D(dev);
++	free_netdev(dev);
++}
++
+C(...)
+{
+	<...
+-	netdev_set_priv_destructor(NDEV, D);
++	netdev_set_priv_destructor(NDEV, E2);
+	...>
+}
-- 
1.9.1

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Johannes Berg]

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.

I think we may need to worry about more kernel versions than just 4.12
here, since this stuff is getting backported, and then it would cause
double-frees.

I'm also considering simply removing the priv_destructor entirely from
mac80211, but I guess this would still apply to other drivers.

johannes

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Arend van Spriel]

On 6/12/2017 11:11 AM, 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.
> 
> I think we may need to worry about more kernel versions than just 4.12
> here, since this stuff is getting backported, and then it would cause
> double-frees.
> 
> I'm also considering simply removing the priv_destructor entirely from
> mac80211, but I guess this would still apply to other drivers.

For current backports the only other driver is brcmfmac. So you don't 
need it in mac80211? Does that mean you move stuff to .ndo_uninit()?

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Johannes Berg]

On Mon, 2017-06-12 at 11:15 +0200, Arend van Spriel wrote:
> On 6/12/2017 11:11 AM, 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.
> > 
> > I think we may need to worry about more kernel versions than just
> > 4.12
> > here, since this stuff is getting backported, and then it would
> > cause
> > double-frees.
> > 
> > I'm also considering simply removing the priv_destructor entirely
> > from
> > mac80211, but I guess this would still apply to other drivers.
> 
> For current backports the only other driver is brcmfmac. 

Right.

> So you don't 
> need it in mac80211? Does that mean you move stuff to .ndo_uninit()?

No, just call more things explicitly and remove destructor usage
entirely. See the thread with davem?

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Johannes Berg]

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?

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Arend van Spriel]

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. So are you still considering the patch
despite the likely backport of commit cf124db566e6. I can do my homework
better and resubmit if needed.

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Johannes Berg]

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.

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Arend van Spriel]

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
---
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;
 @@

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

Re: [PATCH] backport: handle change in netdevice destructor usage

[Johannes Berg]

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