| Message ID | 1497650261-12918-1-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 6/16/17, 2:57 PM, "ovs-dev-bounces@openvswitch.org on behalf of Greg Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com> wrote:
The attribute __ro_after_init was introduced in Linux kernel 4.5. If
a data structure is given this attribute then after the driver module
loads the memory page where the data resides will be marked read only.
The compat code in cache.h always defines __ro_after_init if it is not
already defined so that it can be used as an attribute for the datapath
genl_family structure definitions. If __ro_after_init is defined then
it is used "as-is" where it will apply the read only attribute after
driver initialization.
This is incorrect usage for the Generic Netlink genl_family structure
definitions prior to Linux kernel 4.10. The genl_family structure
in those kernels includes a list header member that will be written
to when the generic netlink family is unregistered. This will cause
a subsequent page fault and kernel panic because at this time the
genl_family structure data has been marked read only in the page
descriptor.
A new compat macro is introduced in acinclude.m4 to detect when the
genl_family structure has the family_list list header as a member.
In this case HAVE_GENL_FAMILY_LIST is defined and if __ro_after_init
is also defined then it is undefined and redefined as empty. This
will prevent the genl_family data structure from being marked read
only in kernels 4.5 through 4.9 and thus prevent the page fault when
the generic netlink families in datapath.c are unregistered.
V2 - Remember Fixes and Signed-off-by tags this time.
Versioning should go after the 3 dashes “---”
You could add CC: Jarno Rajahalme jarno@ovn.org
near fixes.
Fixes: ba63fe260bd5 ("datapath: Allow compile against current net-next.")
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
acinclude.m4 | 3 +++
datapath/linux/compat/include/linux/cache.h | 9 ++++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 22cb897..2145ecc 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -736,6 +736,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h],
[nf_defrag_ipv6_enable], [net],
[OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])])
+ OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [family_list],
+ [OVS_DEFINE([HAVE_GENL_FAMILY_LIST])])
+
if cmp -s datapath/linux/kcompat.h.new \
datapath/linux/kcompat.h >/dev/null 2>&1; then
diff --git a/datapath/linux/compat/include/linux/cache.h b/datapath/linux/compat/include/linux/cache.h
index 917defa..35da4e7 100644
--- a/datapath/linux/compat/include/linux/cache.h
+++ b/datapath/linux/compat/include/linux/cache.h
@@ -3,8 +3,15 @@
#include_next <linux/cache.h>
+#ifdef HAVE_GENL_FAMILY_LIST
+#ifdef __ro_after_init
+#undef __ro_after_init
+#endif /* #ifdef __ro_after_init */
+#define __ro_after_init
+#else
#ifndef __ro_after_init
#define __ro_after_init
-#endif
+#endif /* #ifndef __ro_after_init */
+#endif /* #ifdef HAVE_GENL_FAMILY_LIST */
#endif
--
1.8.3.1
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=34TzOeGOTbwYvq51XZhtCI_rKXQ7eZ_I9T_ISZzfEfY&s=XxSBfvaWXpSRXoew6_SMx7joISTqgZTLqMYK1ZpMn0c&e=
On 06/16/2017 04:15 PM, Darrell Ball wrote: > > > On 6/16/17, 2:57 PM, "ovs-dev-bounces@openvswitch.org on behalf of Greg Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com> wrote: > > The attribute __ro_after_init was introduced in Linux kernel 4.5. If > a data structure is given this attribute then after the driver module > loads the memory page where the data resides will be marked read only. > > The compat code in cache.h always defines __ro_after_init if it is not > already defined so that it can be used as an attribute for the datapath > genl_family structure definitions. If __ro_after_init is defined then > it is used "as-is" where it will apply the read only attribute after > driver initialization. > > This is incorrect usage for the Generic Netlink genl_family structure > definitions prior to Linux kernel 4.10. The genl_family structure > in those kernels includes a list header member that will be written > to when the generic netlink family is unregistered. This will cause > a subsequent page fault and kernel panic because at this time the > genl_family structure data has been marked read only in the page > descriptor. > > A new compat macro is introduced in acinclude.m4 to detect when the > genl_family structure has the family_list list header as a member. > In this case HAVE_GENL_FAMILY_LIST is defined and if __ro_after_init > is also defined then it is undefined and redefined as empty. This > will prevent the genl_family data structure from being marked read > only in kernels 4.5 through 4.9 and thus prevent the page fault when > the generic netlink families in datapath.c are unregistered. > > V2 - Remember Fixes and Signed-off-by tags this time. > > Versioning should go after the 3 dashes “---” Right... still stuck in my old Intel habits where Jeff Kirsher was responsible for stripping that out. Thanks for the reminder. > You could add CC: Jarno Rajahalme jarno@ovn.org > near fixes. > OK, will do. V3 coming up. Thanks for the review Darrell! - Greg > > Fixes: ba63fe260bd5 ("datapath: Allow compile against current net-next.") > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > --- > acinclude.m4 | 3 +++ > datapath/linux/compat/include/linux/cache.h | 9 ++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/acinclude.m4 b/acinclude.m4 > index 22cb897..2145ecc 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -736,6 +736,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h], > [nf_defrag_ipv6_enable], [net], > [OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])]) > + OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [family_list], > + [OVS_DEFINE([HAVE_GENL_FAMILY_LIST])]) > + > > if cmp -s datapath/linux/kcompat.h.new \ > datapath/linux/kcompat.h >/dev/null 2>&1; then > diff --git a/datapath/linux/compat/include/linux/cache.h b/datapath/linux/compat/include/linux/cache.h > index 917defa..35da4e7 100644 > --- a/datapath/linux/compat/include/linux/cache.h > +++ b/datapath/linux/compat/include/linux/cache.h > @@ -3,8 +3,15 @@ > > #include_next <linux/cache.h> > > +#ifdef HAVE_GENL_FAMILY_LIST > +#ifdef __ro_after_init > +#undef __ro_after_init > +#endif /* #ifdef __ro_after_init */ > +#define __ro_after_init > +#else > #ifndef __ro_after_init > #define __ro_after_init > -#endif > +#endif /* #ifndef __ro_after_init */ > +#endif /* #ifdef HAVE_GENL_FAMILY_LIST */ > > #endif > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=34TzOeGOTbwYvq51XZhtCI_rKXQ7eZ_I9T_ISZzfEfY&s=XxSBfvaWXpSRXoew6_SMx7joISTqgZTLqMYK1ZpMn0c&e= > >
diff --git a/acinclude.m4 b/acinclude.m4 index 22cb897..2145ecc 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -736,6 +736,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h], [nf_defrag_ipv6_enable], [net], [OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])]) + OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [family_list], + [OVS_DEFINE([HAVE_GENL_FAMILY_LIST])]) + if cmp -s datapath/linux/kcompat.h.new \ datapath/linux/kcompat.h >/dev/null 2>&1; then diff --git a/datapath/linux/compat/include/linux/cache.h b/datapath/linux/compat/include/linux/cache.h index 917defa..35da4e7 100644 --- a/datapath/linux/compat/include/linux/cache.h +++ b/datapath/linux/compat/include/linux/cache.h @@ -3,8 +3,15 @@ #include_next <linux/cache.h> +#ifdef HAVE_GENL_FAMILY_LIST +#ifdef __ro_after_init +#undef __ro_after_init +#endif /* #ifdef __ro_after_init */ +#define __ro_after_init +#else #ifndef __ro_after_init #define __ro_after_init -#endif +#endif /* #ifndef __ro_after_init */ +#endif /* #ifdef HAVE_GENL_FAMILY_LIST */ #endif
The attribute __ro_after_init was introduced in Linux kernel 4.5. If a data structure is given this attribute then after the driver module loads the memory page where the data resides will be marked read only. The compat code in cache.h always defines __ro_after_init if it is not already defined so that it can be used as an attribute for the datapath genl_family structure definitions. If __ro_after_init is defined then it is used "as-is" where it will apply the read only attribute after driver initialization. This is incorrect usage for the Generic Netlink genl_family structure definitions prior to Linux kernel 4.10. The genl_family structure in those kernels includes a list header member that will be written to when the generic netlink family is unregistered. This will cause a subsequent page fault and kernel panic because at this time the genl_family structure data has been marked read only in the page descriptor. A new compat macro is introduced in acinclude.m4 to detect when the genl_family structure has the family_list list header as a member. In this case HAVE_GENL_FAMILY_LIST is defined and if __ro_after_init is also defined then it is undefined and redefined as empty. This will prevent the genl_family data structure from being marked read only in kernels 4.5 through 4.9 and thus prevent the page fault when the generic netlink families in datapath.c are unregistered. V2 - Remember Fixes and Signed-off-by tags this time. Fixes: ba63fe260bd5 ("datapath: Allow compile against current net-next.") Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- acinclude.m4 | 3 +++ datapath/linux/compat/include/linux/cache.h | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)