| Message ID | 1498078699-27809-1-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2017-06-21 at 21:58 +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."). > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- > Hi Johannes, > > It took a while for me to be happy with this version. More so because > I > found that brcmfmac has a double-free bug if register_netdevice() > fails > so I needed to solve that puzzle. Below is the changelog. > > Regards, > Arend > > V2: > - changed netdev_set_priv_destructor() to macro. > - revised semantic patch covering drivers only needing > free_netdev(). > - used some tricks suggested by Julia in semantic patch. > - added more version checks against 4.13. > --- > backport/backport-include/linux/netdevice.h | 9 ++++ > patches/0079-netdev-destructor.cocci | 84 > +++++++++++++++++++++++++++++ > 2 files changed, 93 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..0f5b198 100644 > --- a/backport/backport-include/linux/netdevice.h > +++ b/backport/backport-include/linux/netdevice.h > @@ -320,4 +320,13 @@ static inline void netif_trans_update(struct > net_device *dev) > } > #endif > > +#if LINUX_VERSION_IS_LESS(4,13,0) > +#define netdev_set_priv_destructor(_dev, _destructor) \ > + (_dev)->destructor = __ ## _destructor > +#else Won't this cause annoying warnings, that __##_destructor is an unused function, when compiling for higher kernels? johannes -- To unsubscribe from this list: send the line "unsubscribe backports" in
Perhaps that can be fixed as such: > +@r2 depends on r1@ > +identifier r1.D, r1.C; > +fresh identifier E = "__" ## D; attribute __maybe_unused; > +@@ > + > ++#if LINUX_VERSION_IS_LESS(4,13,0) > ++static void E(struct net_device *ndev) __maybe_unused johannes -- To unsubscribe from this list: send the line "unsubscribe backports" in
On 21-06-17 23:18, Johannes Berg wrote: > Perhaps that can be fixed as such: > >> +@r2 depends on r1@ >> +identifier r1.D, r1.C; >> +fresh identifier E = "__" ## D; > > attribute __maybe_unused; > >> +@@ >> + >> ++#if LINUX_VERSION_IS_LESS(4,13,0) >> ++static void E(struct net_device *ndev) > > __maybe_unused Ok. I clearly made a mistake in #else of the macro: +#if LINUX_VERSION_IS_LESS(4,13,0) +#define netdev_set_priv_destructor(_dev, _destructor) \ + (_dev)->destructor = __ ## _destructor +#else + (_dev)->needs_free_netdev = true; + if (_destructor != free_netdev) + (_dev)->priv_destructor = _destructor; +#endif So I will fix that, but I do not quite understand your comment. Expanding the macro in spatch output for net/mac80211/iface.c: +#if LINUX_VERSION_IS_LESS(4,13,0) +static void __ieee80211_if_free(struct net_device *ndev) +{ + ieee80211_if_free(ndev); + free_netdev(ndev); +} +#endif + static void ieee80211_if_setup(struct net_device *dev) { ether_setup(dev); dev->priv_flags &= ~IFF_TX_SKB_SHARING; dev->netdev_ops = &ieee80211_dataif_ops; - dev->needs_free_netdev = true; - dev->priv_destructor = ieee80211_if_free; +#if LINUX_VERSION_IS_LESS(4,13,0) + dev->destructor = __ieee80211_if_free +#else + dev->needs_free_netdev = true; + if (ieee80211_if_free != free_netdev) + dev->priv_destructor = ieee80211_if_free; +#endif } So __ieee80211_if_free is only defined for lower kernels and it is only used for lower kernels. Adding the __ieee80211_if_free depends on r1 in the semantic patch. So I will send a V3 fixing the macro in netdevice.h, but I don't think we need the maybe_unused attribute. Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe backports" in
On Wed, 2017-06-21 at 23:50 +0200, Arend van Spriel wrote: > > > > ++#if LINUX_VERSION_IS_LESS(4,13,0) > > > ++static void E(struct net_device *ndev) > So I will send a V3 fixing the macro in netdevice.h, but I don't > think we need the maybe_unused attribute. Ummm, yeah, I totally missed the #if above :) Maybe it's time to get some sleep... johannes -- To unsubscribe from this list: send the line "unsubscribe backports" in
On 21-06-17 23:52, Johannes Berg wrote: > On Wed, 2017-06-21 at 23:50 +0200, Arend van Spriel wrote: >> >>>> ++#if LINUX_VERSION_IS_LESS(4,13,0) >>>> ++static void E(struct net_device *ndev) > >> So I will send a V3 fixing the macro in netdevice.h, but I don't >> think we need the maybe_unused attribute. > > Ummm, yeah, I totally missed the #if above :) > > Maybe it's time to get some sleep... Dito. That V3 will have to wait until tomorrow. Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe backports" in
diff --git a/backport/backport-include/linux/netdevice.h b/backport/backport-include/linux/netdevice.h index 06230b5..0f5b198 100644 --- a/backport/backport-include/linux/netdevice.h +++ b/backport/backport-include/linux/netdevice.h @@ -320,4 +320,13 @@ static inline void netif_trans_update(struct net_device *dev) } #endif +#if LINUX_VERSION_IS_LESS(4,13,0) +#define netdev_set_priv_destructor(_dev, _destructor) \ + (_dev)->destructor = __ ## _destructor +#else + (_dev)->needs_free_netdev = true; + if (_destructor != free_netdev) + (_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..fab8af1 --- /dev/null +++ b/patches/0079-netdev-destructor.cocci @@ -0,0 +1,84 @@ +@r1@ +struct net_device *NDEV; +identifier D, C; +identifier TRUE =~ "true"; +@@ +C(...) +{ + <... +- NDEV->needs_free_netdev = TRUE; +- NDEV->priv_destructor = D; ++ netdev_set_priv_destructor(NDEV, D); + ...> +} + +@r2 depends on r1@ +identifier r1.D, r1.C; +fresh identifier E = "__" ## D; +@@ + ++#if LINUX_VERSION_IS_LESS(4,13,0) ++static void E(struct net_device *ndev) ++{ ++ D(ndev); ++ free_netdev(ndev); ++} ++#endif ++ +C(...) +{ + ... +} + +@r3 depends on r1@ +type T; +identifier NDEV; +identifier r1.D; +T RET; +@@ + +RET = \(register_netdevice\|register_ndev\)(NDEV); +if (<+... RET ...+>) { + <... ++#if LINUX_VERSION_IS_LESS(4,13,0) ++ D(NDEV); ++#endif + free_netdev(NDEV); + ...> +} + +@r4 depends on r1@ +identifier NDEV; +identifier r1.D; +type T; +T RET; +@@ + +if (...) + RET = register_netdevice(NDEV); +else + RET = register_netdev(NDEV); +if (<+... RET ...+>) { + <... ++#if LINUX_VERSION_IS_LESS(4,13,0) ++ D(NDEV); ++#endif + free_netdev(NDEV); + ...> +} + +@r5@ +struct net_device *NDEV; +identifier TRUE =~ "true"; +@@ + +-NDEV->needs_free_netdev = TRUE; ++netdev_set_priv_destructor(NDEV, free_netdev); + +@r6@ +struct net_device *NDEV; +identifier D; +@@ + +-NDEV->priv_destructor = D; ++netdev_set_priv_destructor(NDEV, D);
This patch deals with changes made in struct net_device by commit cf124db566e6 ("net: Fix inconsistent teardown and release of private netdev state."). Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> --- Hi Johannes, It took a while for me to be happy with this version. More so because I found that brcmfmac has a double-free bug if register_netdevice() fails so I needed to solve that puzzle. Below is the changelog. Regards, Arend V2: - changed netdev_set_priv_destructor() to macro. - revised semantic patch covering drivers only needing free_netdev(). - used some tricks suggested by Julia in semantic patch. - added more version checks against 4.13. --- backport/backport-include/linux/netdevice.h | 9 ++++ patches/0079-netdev-destructor.cocci | 84 +++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 patches/0079-netdev-destructor.cocci