| Message ID | CAPWQB7Ed_7Ze+WXg8J3E9+Hd_xe+gB8v+uPxua6WfW5QQnOhyw@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On 19 June 2017 at 11:44, Joe Stringer <joe@ovn.org> wrote: > On 16 June 2017 at 16:37, Greg Rose <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. >> >> Fixes: ba63fe260bd5 ("datapath: Allow compile against current net-next.") >> CC: Jarno Rajahalme <jarno@ovn.org> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> --- > > Nice find, thanks for the patch! > > Sometimes it's nice to have some of this context from the commit > message above the macro define. I'm considering to roll this > incremental into the patch before applying to master shortly: > > diff --git a/datapath/linux/compat/include/linux/cache.h > b/datapath/linux/compat/include/linux/cache.h > index 35da4e70ce5c..87f543722b62 100644 > --- a/datapath/linux/compat/include/linux/cache.h > +++ b/datapath/linux/compat/include/linux/cache.h > @@ -3,6 +3,12 @@ > > #include_next <linux/cache.h> > > +/* Commit c74ba8b3480d ("arch: Introduce post-init read-only memory") > + * introduced the __ro_after_init attribute, however it wasn't applied to > + * generic netlink sockets until commit 34158151d2aa ("netfilter: cttimeout: > + * use nf_ct_iterate_cleanup_net to unlink timeout objs"). Using it on > + * genetlink before the latter commit leads to crash on module unload. > + * For kernels < 4.10, define it as empty. */ > #ifdef HAVE_GENL_FAMILY_LIST > #ifdef __ro_after_init > #undef __ro_after_init Applied to master and branch-2.7.
On 06/19/2017 01:36 PM, Joe Stringer wrote: > On 19 June 2017 at 11:44, Joe Stringer <joe@ovn.org> wrote: > > On 16 June 2017 at 16:37, Greg Rose <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. > >> > >> Fixes: ba63fe260bd5 ("datapath: Allow compile against current net-next.") > >> CC: Jarno Rajahalme <jarno@ovn.org> > >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> > >> --- > > > > Nice find, thanks for the patch! > > > > Sometimes it's nice to have some of this context from the commit > > message above the macro define. I'm considering to roll this > > incremental into the patch before applying to master shortly: > > > > diff --git a/datapath/linux/compat/include/linux/cache.h > > b/datapath/linux/compat/include/linux/cache.h > > index 35da4e70ce5c..87f543722b62 100644 > > --- a/datapath/linux/compat/include/linux/cache.h > > +++ b/datapath/linux/compat/include/linux/cache.h > > @@ -3,6 +3,12 @@ > > > > #include_next <linux/cache.h> > > > > +/* Commit c74ba8b3480d ("arch: Introduce post-init read-only memory") > > + * introduced the __ro_after_init attribute, however it wasn't applied to > > + * generic netlink sockets until commit 34158151d2aa ("netfilter: cttimeout: > > + * use nf_ct_iterate_cleanup_net to unlink timeout objs"). Using it on > > + * genetlink before the latter commit leads to crash on module unload. > > + * For kernels < 4.10, define it as empty. */ > > #ifdef HAVE_GENL_FAMILY_LIST > > #ifdef __ro_after_init > > #undef __ro_after_init > > Applied to master and branch-2.7. > Thank you Joe! And the incremental patch is helpful so thanks for that as well. - Greg
diff --git a/datapath/linux/compat/include/linux/cache.h b/datapath/linux/compat/include/linux/cache.h index 35da4e70ce5c..87f543722b62 100644 --- a/datapath/linux/compat/include/linux/cache.h +++ b/datapath/linux/compat/include/linux/cache.h @@ -3,6 +3,12 @@ #include_next <linux/cache.h> +/* Commit c74ba8b3480d ("arch: Introduce post-init read-only memory") + * introduced the __ro_after_init attribute, however it wasn't applied to + * generic netlink sockets until commit 34158151d2aa ("netfilter: cttimeout: + * use nf_ct_iterate_cleanup_net to unlink timeout objs"). Using it on + * genetlink before the latter commit leads to crash on module unload. + * For kernels < 4.10, define it as empty. */ #ifdef HAVE_GENL_FAMILY_LIST #ifdef __ro_after_init #undef __ro_after_init