| Message ID | 1532051311-9749-1-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] compat: Initialize IPv4 reassembly secret timer | expand |
On 7/19/2018 6:48 PM, Greg Rose wrote: > The RHEL 7 kernels expect the secret timer interval to be initialized > before calling the inet_frags_init() function. By not initializing it > the inet_frags_secret_rebuild() function was running on every tick > rather than on the expected interval. This caused occasional panics > from page faults when inet_frags_secret_rebuild() would try to rearm a > timer from the openvswitch kernel module which had just been removed. > > Also remove the prior, and now unnecessary, work around. > > VMware BZ 2094203 > > Fixes: 595e069a ("compat: Backport IPv4 reassembly.") > Signed-off-by: Greg Rose <gvrose8192@gmail.com> Copying Pravin and Flavio as well. - Greg > --- > datapath/datapath.c | 10 ---------- > datapath/linux/compat/ip_fragment.c | 3 +++ > datapath/linux/compat/nf_conntrack_reasm.c | 3 +++ > 3 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/datapath/datapath.c b/datapath/datapath.c > index 43f0d74..3ea240a 100644 > --- a/datapath/datapath.c > +++ b/datapath/datapath.c > @@ -2478,16 +2478,6 @@ error: > > static void dp_cleanup(void) > { > -#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > - /* On RHEL 7.x kernels we hit a kernel paging error without > - * this barrier and subsequent hefty delay. A process will > - * attempt to access openvwitch memory after it has been > - * unloaded. Further debugging is needed on that but for > - * now let's not let customer machines panic. > - */ > - rcu_barrier(); > - msleep(3000); > -#endif > dp_unregister_genl(ARRAY_SIZE(dp_genl_families)); > ovs_netdev_exit(); > unregister_netdevice_notifier(&ovs_dp_device_notifier); > diff --git a/datapath/linux/compat/ip_fragment.c b/datapath/linux/compat/ip_fragment.c > index 8f2012b..f910b99 100644 > --- a/datapath/linux/compat/ip_fragment.c > +++ b/datapath/linux/compat/ip_fragment.c > @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) > #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK > ip4_frags.frags_cache_name = ip_frag_cache_name; > #endif > +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > + ip4_frags.secret_interval = 10 * 60 * HZ; > +#endif > if (inet_frags_init(&ip4_frags)) { > pr_warn("IP: failed to allocate ip4_frags cache\n"); > return -ENOMEM; > diff --git a/datapath/linux/compat/nf_conntrack_reasm.c b/datapath/linux/compat/nf_conntrack_reasm.c > index ea153c3..ce13112 100644 > --- a/datapath/linux/compat/nf_conntrack_reasm.c > +++ b/datapath/linux/compat/nf_conntrack_reasm.c > @@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void) > #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK > nf_frags.frags_cache_name = nf_frags_cache_name; > #endif > +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > + nf_frags.secret_interval = 10 * 60 * HZ; > +#endif > ret = inet_frags_init(&nf_frags); > if (ret) > goto out;
On Thu, Jul 19, 2018 at 6:48 PM, Greg Rose <gvrose8192@gmail.com> wrote: > The RHEL 7 kernels expect the secret timer interval to be initialized > before calling the inet_frags_init() function. By not initializing it > the inet_frags_secret_rebuild() function was running on every tick > rather than on the expected interval. This caused occasional panics > from page faults when inet_frags_secret_rebuild() would try to rearm a > timer from the openvswitch kernel module which had just been removed. Thanks Greg for the patch. The bug hides much deeper than I would expect. > Also remove the prior, and now unnecessary, work around. > > VMware BZ 2094203 A minor nit, maybe VMware-BZ: #2094203 > --- a/datapath/linux/compat/ip_fragment.c > +++ b/datapath/linux/compat/ip_fragment.c > @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) > #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK > ip4_frags.frags_cache_name = ip_frag_cache_name; > #endif > +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > + ip4_frags.secret_interval = 10 * 60 * HZ; > +#endif This is a great catch! Looks like we remove the secret_interval in net-next commit e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") But somehow RHEL kernel still retain this field, and the secret_interval was set to 10 * 60 * HZ. I would recommend to set 'secret_interval' by grepping this filed in 'struct inet_frags' rather than by kernel version. So that if RHEL 8 kernel still have this field, your fix will apply. > --- a/datapath/linux/compat/nf_conntrack_reasm.c > +++ b/datapath/linux/compat/nf_conntrack_reasm.c > @@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void) > #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK > nf_frags.frags_cache_name = nf_frags_cache_name; > #endif > +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > + nf_frags.secret_interval = 10 * 60 * HZ; > +#endif Same for here. -Yi-Hung
On 7/20/2018 2:00 PM, Yi-Hung Wei wrote: > On Thu, Jul 19, 2018 at 6:48 PM, Greg Rose <gvrose8192@gmail.com> wrote: >> The RHEL 7 kernels expect the secret timer interval to be initialized >> before calling the inet_frags_init() function. By not initializing it >> the inet_frags_secret_rebuild() function was running on every tick >> rather than on the expected interval. This caused occasional panics >> from page faults when inet_frags_secret_rebuild() would try to rearm a >> timer from the openvswitch kernel module which had just been removed. > Thanks Greg for the patch. The bug hides much deeper than I would expect. > >> Also remove the prior, and now unnecessary, work around. >> >> VMware BZ 2094203 > A minor nit, maybe VMware-BZ: #2094203 I'm fine with that. I wasn't aware of any specific format. > >> --- a/datapath/linux/compat/ip_fragment.c >> +++ b/datapath/linux/compat/ip_fragment.c >> @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) >> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK >> ip4_frags.frags_cache_name = ip_frag_cache_name; >> #endif >> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) >> + ip4_frags.secret_interval = 10 * 60 * HZ; >> +#endif > This is a great catch! Looks like we remove the secret_interval in > net-next commit > e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") > But somehow RHEL kernel still retain this field, and the > secret_interval was set to 10 * 60 * HZ. > > I would recommend to set 'secret_interval' by grepping this filed in > 'struct inet_frags' rather than by kernel version. So that if RHEL 8 > kernel still have this field, your fix will apply. That's a good point to raise. My thinking was to specifically call it out for RHEL 7 because it hasn't been in the kernel since 3.16 and this bug is RHEL 7 specific. That's a long way back and I'm sure it will not be there in the RHEL 8 kernel. This makes it clear that it is only for RHEL 7 and nothing else. However, if you and others feel strongly about it I don't mind making the change. > >> --- a/datapath/linux/compat/nf_conntrack_reasm.c >> +++ b/datapath/linux/compat/nf_conntrack_reasm.c >> @@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void) >> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK >> nf_frags.frags_cache_name = nf_frags_cache_name; >> #endif >> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) >> + nf_frags.secret_interval = 10 * 60 * HZ; >> +#endif > Same for here. > > -Yi-Hung Thanks for the review! I'll wait another day or two to get more feedback. - Greg
>>> --- a/datapath/linux/compat/ip_fragment.c >>> +++ b/datapath/linux/compat/ip_fragment.c >>> @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) >>> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK >>> ip4_frags.frags_cache_name = ip_frag_cache_name; >>> #endif >>> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) >>> + ip4_frags.secret_interval = 10 * 60 * HZ; >>> +#endif >> >> This is a great catch! Looks like we remove the secret_interval in >> net-next commit >> e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") >> But somehow RHEL kernel still retain this field, and the >> secret_interval was set to 10 * 60 * HZ. >> >> I would recommend to set 'secret_interval' by grepping this filed in >> 'struct inet_frags' rather than by kernel version. So that if RHEL 8 >> kernel still have this field, your fix will apply. > > > That's a good point to raise. > > My thinking was to specifically call it out for RHEL 7 because it hasn't > been in the kernel since 3.16 > and this bug is RHEL 7 specific. That's a long way back and I'm sure it will > not be there in > the RHEL 8 kernel. This makes it clear that it is only for RHEL 7 and > nothing else. > > However, if you and others feel strongly about it I don't mind making the > change. I do not have a strong opinion no this. I think current solution also work. Thanks, Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
On 7/23/2018 9:22 AM, Yi-Hung Wei wrote: >>>> --- a/datapath/linux/compat/ip_fragment.c >>>> +++ b/datapath/linux/compat/ip_fragment.c >>>> @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) >>>> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK >>>> ip4_frags.frags_cache_name = ip_frag_cache_name; >>>> #endif >>>> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) >>>> + ip4_frags.secret_interval = 10 * 60 * HZ; >>>> +#endif >>> This is a great catch! Looks like we remove the secret_interval in >>> net-next commit >>> e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") >>> But somehow RHEL kernel still retain this field, and the >>> secret_interval was set to 10 * 60 * HZ. >>> >>> I would recommend to set 'secret_interval' by grepping this filed in >>> 'struct inet_frags' rather than by kernel version. So that if RHEL 8 >>> kernel still have this field, your fix will apply. >> >> That's a good point to raise. >> >> My thinking was to specifically call it out for RHEL 7 because it hasn't >> been in the kernel since 3.16 >> and this bug is RHEL 7 specific. That's a long way back and I'm sure it will >> not be there in >> the RHEL 8 kernel. This makes it clear that it is only for RHEL 7 and >> nothing else. >> >> However, if you and others feel strongly about it I don't mind making the >> change. > I do not have a strong opinion no this. I think current solution also work. > > Thanks, > > Acked-by: Yi-Hung Wei <yihung.wei@gmail.com> Is there anything else required for this patch to get it committed? Thanks, - Greg
On Wed, Jul 25, 2018 at 08:46:21AM -0700, Gregory Rose wrote: > On 7/23/2018 9:22 AM, Yi-Hung Wei wrote: > >>>>--- a/datapath/linux/compat/ip_fragment.c > >>>>+++ b/datapath/linux/compat/ip_fragment.c > >>>>@@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) > >>>> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK > >>>> ip4_frags.frags_cache_name = ip_frag_cache_name; > >>>> #endif > >>>>+#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) > >>>>+ ip4_frags.secret_interval = 10 * 60 * HZ; > >>>>+#endif > >>>This is a great catch! Looks like we remove the secret_interval in > >>>net-next commit > >>>e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") > >>>But somehow RHEL kernel still retain this field, and the > >>>secret_interval was set to 10 * 60 * HZ. > >>> > >>>I would recommend to set 'secret_interval' by grepping this filed in > >>>'struct inet_frags' rather than by kernel version. So that if RHEL 8 > >>>kernel still have this field, your fix will apply. > >> > >>That's a good point to raise. > >> > >>My thinking was to specifically call it out for RHEL 7 because it hasn't > >>been in the kernel since 3.16 > >>and this bug is RHEL 7 specific. That's a long way back and I'm sure it will > >>not be there in > >>the RHEL 8 kernel. This makes it clear that it is only for RHEL 7 and > >>nothing else. > >> > >>However, if you and others feel strongly about it I don't mind making the > >>change. > >I do not have a strong opinion no this. I think current solution also work. > > > >Thanks, > > > >Acked-by: Yi-Hung Wei <yihung.wei@gmail.com> > > Is there anything else required for this patch to get it committed? This didn't attract more reviews so I applied it to master and backported as far as branch-2.5. Thanks, Ben.
On 7/27/2018 11:38 AM, Ben Pfaff wrote: > On Wed, Jul 25, 2018 at 08:46:21AM -0700, Gregory Rose wrote: >> On 7/23/2018 9:22 AM, Yi-Hung Wei wrote: >>>>>> --- a/datapath/linux/compat/ip_fragment.c >>>>>> +++ b/datapath/linux/compat/ip_fragment.c >>>>>> @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) >>>>>> #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK >>>>>> ip4_frags.frags_cache_name = ip_frag_cache_name; >>>>>> #endif >>>>>> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) >>>>>> + ip4_frags.secret_interval = 10 * 60 * HZ; >>>>>> +#endif >>>>> This is a great catch! Looks like we remove the secret_interval in >>>>> net-next commit >>>>> e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer") >>>>> But somehow RHEL kernel still retain this field, and the >>>>> secret_interval was set to 10 * 60 * HZ. >>>>> >>>>> I would recommend to set 'secret_interval' by grepping this filed in >>>>> 'struct inet_frags' rather than by kernel version. So that if RHEL 8 >>>>> kernel still have this field, your fix will apply. >>>> That's a good point to raise. >>>> >>>> My thinking was to specifically call it out for RHEL 7 because it hasn't >>>> been in the kernel since 3.16 >>>> and this bug is RHEL 7 specific. That's a long way back and I'm sure it will >>>> not be there in >>>> the RHEL 8 kernel. This makes it clear that it is only for RHEL 7 and >>>> nothing else. >>>> >>>> However, if you and others feel strongly about it I don't mind making the >>>> change. >>> I do not have a strong opinion no this. I think current solution also work. >>> >>> Thanks, >>> >>> Acked-by: Yi-Hung Wei <yihung.wei@gmail.com> >> Is there anything else required for this patch to get it committed? > This didn't attract more reviews so I applied it to master and > backported as far as branch-2.5. > > Thanks, > > Ben. Excellent. Thanks Ben! - Greg
diff --git a/datapath/datapath.c b/datapath/datapath.c index 43f0d74..3ea240a 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -2478,16 +2478,6 @@ error: static void dp_cleanup(void) { -#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) - /* On RHEL 7.x kernels we hit a kernel paging error without - * this barrier and subsequent hefty delay. A process will - * attempt to access openvwitch memory after it has been - * unloaded. Further debugging is needed on that but for - * now let's not let customer machines panic. - */ - rcu_barrier(); - msleep(3000); -#endif dp_unregister_genl(ARRAY_SIZE(dp_genl_families)); ovs_netdev_exit(); unregister_netdevice_notifier(&ovs_dp_device_notifier); diff --git a/datapath/linux/compat/ip_fragment.c b/datapath/linux/compat/ip_fragment.c index 8f2012b..f910b99 100644 --- a/datapath/linux/compat/ip_fragment.c +++ b/datapath/linux/compat/ip_fragment.c @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void) #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK ip4_frags.frags_cache_name = ip_frag_cache_name; #endif +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) + ip4_frags.secret_interval = 10 * 60 * HZ; +#endif if (inet_frags_init(&ip4_frags)) { pr_warn("IP: failed to allocate ip4_frags cache\n"); return -ENOMEM; diff --git a/datapath/linux/compat/nf_conntrack_reasm.c b/datapath/linux/compat/nf_conntrack_reasm.c index ea153c3..ce13112 100644 --- a/datapath/linux/compat/nf_conntrack_reasm.c +++ b/datapath/linux/compat/nf_conntrack_reasm.c @@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void) #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK nf_frags.frags_cache_name = nf_frags_cache_name; #endif +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0) + nf_frags.secret_interval = 10 * 60 * HZ; +#endif ret = inet_frags_init(&nf_frags); if (ret) goto out;
The RHEL 7 kernels expect the secret timer interval to be initialized before calling the inet_frags_init() function. By not initializing it the inet_frags_secret_rebuild() function was running on every tick rather than on the expected interval. This caused occasional panics from page faults when inet_frags_secret_rebuild() would try to rearm a timer from the openvswitch kernel module which had just been removed. Also remove the prior, and now unnecessary, work around. VMware BZ 2094203 Fixes: 595e069a ("compat: Backport IPv4 reassembly.") Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- datapath/datapath.c | 10 ---------- datapath/linux/compat/ip_fragment.c | 3 +++ datapath/linux/compat/nf_conntrack_reasm.c | 3 +++ 3 files changed, 6 insertions(+), 10 deletions(-)