[ovs-dev] [PATCH V3] compat: Restrict __ro_after_init usage

[Joe Stringer]

On 19 June 2017 at 11:44, Joe Stringer <joe at ovn.org> wrote:
> On 16 June 2017 at 16:37, Greg Rose <gvrose8192 at 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 at ovn.org>
>> Signed-off-by: Greg Rose <gvrose8192 at 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.