[Bionic/Cosmic/Disco/Unstable][SRU][PATCH 0/1] openvswitch: kernel oops destroying interfaces on i386 (LP #1736390)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bionic/Cosmic/Disco/Unstable][SRU][PATCH 0/1] openvswitch: kernel oops destroying interfaces on i386 (LP #1736390)

Juerg Haefliger
The commit referenced in the patch introduced a regression on i386. Simply
adding/deleting an OVS bridge in a loop eventually triggers a crash.
Upstream is looking at the problem (I think) but nothing's been found yet.
In the meantime, disable that logic on i386 to get rid of the crash. Per
upstream, this only results in higher CPU usage and potential buffering
issues.

Juerg Haefliger (1):
  UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit

 include/uapi/linux/openvswitch.h |  2 ++
 net/openvswitch/conntrack.c      | 12 ++++++++++++
 2 files changed, 14 insertions(+)

--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic/Cosmic/Disco/Unstable][SRU][PATCH 1/1] UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit

Juerg Haefliger
BugLink: https://bugs.launchpad.net/bugs/1736390

Openvswitch eventmask support for CT actions was introduced with commit
120645513f55 ("openvswitch: Add eventmask support to CT action."). This
commit introduces a regression on i386 which results in a kernel crash.
Fix that by disabling eventmask support on i386.

Per upstream, the result of *not* having this "will cause additional CPU
use and potential buffering issues for CT event monitors in userspace."

https://lore.kernel.org/lkml/E891AED6-8406-4914-BDC3-92248F77C63B@.../

Fixes: 120645513f55 ("openvswitch: Add eventmask support to CT action.")
Signed-off-by: Juerg Haefliger <[hidden email]>
---
 include/uapi/linux/openvswitch.h |  2 ++
 net/openvswitch/conntrack.c      | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index dcfab5e3b55c..9da2942d7f7e 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -744,7 +744,9 @@ enum ovs_ct_attr {
    related connections. */
  OVS_CT_ATTR_NAT,        /* Nested OVS_NAT_ATTR_* */
  OVS_CT_ATTR_FORCE_COMMIT,  /* No argument */
+#ifndef CONFIG_X86_32
  OVS_CT_ATTR_EVENTMASK,  /* u32 mask of IPCT_* events. */
+#endif
  __OVS_CT_ATTR_MAX
 };
 
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 8f04bd6e44bb..0c44fb56317e 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -66,9 +66,13 @@ struct ovs_conntrack_info {
  u8 commit : 1;
  u8 nat : 3;                 /* enum ovs_ct_nat */
  u8 force : 1;
+#ifndef CONFIG_X86_32
  u8 have_eventmask : 1;
+#endif
  u16 family;
+#ifndef CONFIG_X86_32
  u32 eventmask;              /* Mask of 1 << IPCT_*. */
+#endif
  struct md_mark mark;
  struct md_labels labels;
 #ifdef CONFIG_NF_NAT_NEEDED
@@ -1054,6 +1058,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
  if (!ct)
  return 0;
 
+#ifndef CONFIG_X86_32
  /* Set the conntrack event mask if given.  NEW and DELETE events have
  * their own groups, but the NFNLGRP_CONNTRACK_UPDATE group listener
  * typically would receive many kinds of updates.  Setting the event
@@ -1067,6 +1072,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
  if (cache)
  cache->ctmask = info->eventmask;
  }
+#endif
 
  /* Apply changes before confirming the connection so that the initial
  * conntrack NEW netlink event carries the values given in the CT
@@ -1340,8 +1346,10 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
  /* NAT length is checked when parsing the nested attributes. */
  [OVS_CT_ATTR_NAT] = { .minlen = 0, .maxlen = INT_MAX },
 #endif
+#ifndef CONFIG_X86_32
  [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
     .maxlen = sizeof(u32) },
+#endif
 };
 
 static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
@@ -1423,10 +1431,12 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
  break;
  }
 #endif
+#ifndef CONFIG_X86_32
  case OVS_CT_ATTR_EVENTMASK:
  info->have_eventmask = true;
  info->eventmask = nla_get_u32(a);
  break;
+#endif
 
  default:
  OVS_NLERR(log, "Unknown conntrack attr (%d)",
@@ -1627,9 +1637,11 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
    ct_info->helper->name))
  return -EMSGSIZE;
  }
+#ifndef CONFIG_X86_32
  if (ct_info->have_eventmask &&
     nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))
  return -EMSGSIZE;
+#endif
 
 #ifdef CONFIG_NF_NAT_NEEDED
  if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Bionic/Cosmic/Disco/Unstable][SRU][PATCH 1/1] UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit

Thadeu Lima de Souza Cascardo-3
On Mon, Mar 11, 2019 at 08:36:43AM +0100, Juerg Haefliger wrote:

> BugLink: https://bugs.launchpad.net/bugs/1736390
>
> Openvswitch eventmask support for CT actions was introduced with commit
> 120645513f55 ("openvswitch: Add eventmask support to CT action."). This
> commit introduces a regression on i386 which results in a kernel crash.
> Fix that by disabling eventmask support on i386.
>
> Per upstream, the result of *not* having this "will cause additional CPU
> use and potential buffering issues for CT event monitors in userspace."
>
> https://lore.kernel.org/lkml/E891AED6-8406-4914-BDC3-92248F77C63B@.../
>
> Fixes: 120645513f55 ("openvswitch: Add eventmask support to CT action.")
> Signed-off-by: Juerg Haefliger <[hidden email]>
> ---
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/conntrack.c      | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
>

I have gone through the bug, the ovs thread and the ovs patch, with some of the
explanation from Jarno on why this should only be disabled on i386.

I am still not very comfortable with this, as this seems to be corrupting
memory on different places, by looking at the backtrace you used on the ovs-dev
mailing list.

In any case, I failed to identify the test results with the patch applied on
the bug. Can you tell more about them?

Cascardo.

> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index dcfab5e3b55c..9da2942d7f7e 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -744,7 +744,9 @@ enum ovs_ct_attr {
>     related connections. */
>   OVS_CT_ATTR_NAT,        /* Nested OVS_NAT_ATTR_* */
>   OVS_CT_ATTR_FORCE_COMMIT,  /* No argument */
> +#ifndef CONFIG_X86_32
>   OVS_CT_ATTR_EVENTMASK,  /* u32 mask of IPCT_* events. */
> +#endif
>   __OVS_CT_ATTR_MAX
>  };
>  
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 8f04bd6e44bb..0c44fb56317e 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -66,9 +66,13 @@ struct ovs_conntrack_info {
>   u8 commit : 1;
>   u8 nat : 3;                 /* enum ovs_ct_nat */
>   u8 force : 1;
> +#ifndef CONFIG_X86_32
>   u8 have_eventmask : 1;
> +#endif
>   u16 family;
> +#ifndef CONFIG_X86_32
>   u32 eventmask;              /* Mask of 1 << IPCT_*. */
> +#endif
>   struct md_mark mark;
>   struct md_labels labels;
>  #ifdef CONFIG_NF_NAT_NEEDED
> @@ -1054,6 +1058,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>   if (!ct)
>   return 0;
>  
> +#ifndef CONFIG_X86_32
>   /* Set the conntrack event mask if given.  NEW and DELETE events have
>   * their own groups, but the NFNLGRP_CONNTRACK_UPDATE group listener
>   * typically would receive many kinds of updates.  Setting the event
> @@ -1067,6 +1072,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>   if (cache)
>   cache->ctmask = info->eventmask;
>   }
> +#endif
>  
>   /* Apply changes before confirming the connection so that the initial
>   * conntrack NEW netlink event carries the values given in the CT
> @@ -1340,8 +1346,10 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
>   /* NAT length is checked when parsing the nested attributes. */
>   [OVS_CT_ATTR_NAT] = { .minlen = 0, .maxlen = INT_MAX },
>  #endif
> +#ifndef CONFIG_X86_32
>   [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
>      .maxlen = sizeof(u32) },
> +#endif
>  };
>  
>  static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> @@ -1423,10 +1431,12 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
>   break;
>   }
>  #endif
> +#ifndef CONFIG_X86_32
>   case OVS_CT_ATTR_EVENTMASK:
>   info->have_eventmask = true;
>   info->eventmask = nla_get_u32(a);
>   break;
> +#endif
>  
>   default:
>   OVS_NLERR(log, "Unknown conntrack attr (%d)",
> @@ -1627,9 +1637,11 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
>     ct_info->helper->name))
>   return -EMSGSIZE;
>   }
> +#ifndef CONFIG_X86_32
>   if (ct_info->have_eventmask &&
>      nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))
>   return -EMSGSIZE;
> +#endif
>  
>  #ifdef CONFIG_NF_NAT_NEEDED
>   if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
> --
> 2.19.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Bionic/Cosmic/Disco/Unstable][SRU][PATCH 1/1] UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit

Juerg Haefliger
On Mon, 11 Mar 2019 08:53:22 -0300
Thadeu Lima de Souza Cascardo <[hidden email]> wrote:

> On Mon, Mar 11, 2019 at 08:36:43AM +0100, Juerg Haefliger wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1736390
> >
> > Openvswitch eventmask support for CT actions was introduced with commit
> > 120645513f55 ("openvswitch: Add eventmask support to CT action."). This
> > commit introduces a regression on i386 which results in a kernel crash.
> > Fix that by disabling eventmask support on i386.
> >
> > Per upstream, the result of *not* having this "will cause additional CPU
> > use and potential buffering issues for CT event monitors in userspace."
> >
> > https://lore.kernel.org/lkml/E891AED6-8406-4914-BDC3-92248F77C63B@.../
> >
> > Fixes: 120645513f55 ("openvswitch: Add eventmask support to CT action.")
> > Signed-off-by: Juerg Haefliger <[hidden email]>
> > ---
> >  include/uapi/linux/openvswitch.h |  2 ++
> >  net/openvswitch/conntrack.c      | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
> >  
>
> I have gone through the bug, the ovs thread and the ovs patch, with some of the
> explanation from Jarno on why this should only be disabled on i386.
>
> I am still not very comfortable with this, as this seems to be corrupting
> memory on different places, by looking at the backtrace you used on the ovs-dev
> mailing list.
Yes. That's why I want to disable it completely on i386 until upstream comes
up with a root-cause/fix (which they haven't so far).


> In any case, I failed to identify the test results with the patch applied on
> the bug. Can you tell more about them?

The simple case is to just add and delete an OVS bridge repeatedly which kills
the (i386) machine without this patch.

...Juerg


 

> Cascardo.
>
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index dcfab5e3b55c..9da2942d7f7e 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -744,7 +744,9 @@ enum ovs_ct_attr {
> >     related connections. */
> >   OVS_CT_ATTR_NAT,        /* Nested OVS_NAT_ATTR_* */
> >   OVS_CT_ATTR_FORCE_COMMIT,  /* No argument */
> > +#ifndef CONFIG_X86_32
> >   OVS_CT_ATTR_EVENTMASK,  /* u32 mask of IPCT_* events. */
> > +#endif
> >   __OVS_CT_ATTR_MAX
> >  };
> >  
> > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> > index 8f04bd6e44bb..0c44fb56317e 100644
> > --- a/net/openvswitch/conntrack.c
> > +++ b/net/openvswitch/conntrack.c
> > @@ -66,9 +66,13 @@ struct ovs_conntrack_info {
> >   u8 commit : 1;
> >   u8 nat : 3;                 /* enum ovs_ct_nat */
> >   u8 force : 1;
> > +#ifndef CONFIG_X86_32
> >   u8 have_eventmask : 1;
> > +#endif
> >   u16 family;
> > +#ifndef CONFIG_X86_32
> >   u32 eventmask;              /* Mask of 1 << IPCT_*. */
> > +#endif
> >   struct md_mark mark;
> >   struct md_labels labels;
> >  #ifdef CONFIG_NF_NAT_NEEDED
> > @@ -1054,6 +1058,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> >   if (!ct)
> >   return 0;
> >  
> > +#ifndef CONFIG_X86_32
> >   /* Set the conntrack event mask if given.  NEW and DELETE events have
> >   * their own groups, but the NFNLGRP_CONNTRACK_UPDATE group listener
> >   * typically would receive many kinds of updates.  Setting the event
> > @@ -1067,6 +1072,7 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> >   if (cache)
> >   cache->ctmask = info->eventmask;
> >   }
> > +#endif
> >  
> >   /* Apply changes before confirming the connection so that the initial
> >   * conntrack NEW netlink event carries the values given in the CT
> > @@ -1340,8 +1346,10 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
> >   /* NAT length is checked when parsing the nested attributes. */
> >   [OVS_CT_ATTR_NAT] = { .minlen = 0, .maxlen = INT_MAX },
> >  #endif
> > +#ifndef CONFIG_X86_32
> >   [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
> >      .maxlen = sizeof(u32) },
> > +#endif
> >  };
> >  
> >  static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> > @@ -1423,10 +1431,12 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> >   break;
> >   }
> >  #endif
> > +#ifndef CONFIG_X86_32
> >   case OVS_CT_ATTR_EVENTMASK:
> >   info->have_eventmask = true;
> >   info->eventmask = nla_get_u32(a);
> >   break;
> > +#endif
> >  
> >   default:
> >   OVS_NLERR(log, "Unknown conntrack attr (%d)",
> > @@ -1627,9 +1637,11 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
> >     ct_info->helper->name))
> >   return -EMSGSIZE;
> >   }
> > +#ifndef CONFIG_X86_32
> >   if (ct_info->have_eventmask &&
> >      nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))
> >   return -EMSGSIZE;
> > +#endif
> >  
> >  #ifdef CONFIG_NF_NAT_NEEDED
> >   if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
> > --
> > 2.19.1
> >
> >
> > --
> > kernel-team mailing list
> > [hidden email]
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team 

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Bionic/Cosmic/Disco/Unstable][SRU][PATCH 1/1] UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit

Andrea Righi
On Fri, Apr 05, 2019 at 08:10:42AM +0200, Juerg Haefliger wrote:

> On Mon, 11 Mar 2019 08:53:22 -0300
> Thadeu Lima de Souza Cascardo <[hidden email]> wrote:
>
> > On Mon, Mar 11, 2019 at 08:36:43AM +0100, Juerg Haefliger wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/1736390
> > >
> > > Openvswitch eventmask support for CT actions was introduced with commit
> > > 120645513f55 ("openvswitch: Add eventmask support to CT action."). This
> > > commit introduces a regression on i386 which results in a kernel crash.
> > > Fix that by disabling eventmask support on i386.
> > >
> > > Per upstream, the result of *not* having this "will cause additional CPU
> > > use and potential buffering issues for CT event monitors in userspace."
> > >
> > > https://lore.kernel.org/lkml/E891AED6-8406-4914-BDC3-92248F77C63B@.../
> > >
> > > Fixes: 120645513f55 ("openvswitch: Add eventmask support to CT action.")
> > > Signed-off-by: Juerg Haefliger <[hidden email]>
> > > ---
> > >  include/uapi/linux/openvswitch.h |  2 ++
> > >  net/openvswitch/conntrack.c      | 12 ++++++++++++
> > >  2 files changed, 14 insertions(+)
> > >  
> >
> > I have gone through the bug, the ovs thread and the ovs patch, with some of the
> > explanation from Jarno on why this should only be disabled on i386.
> >
> > I am still not very comfortable with this, as this seems to be corrupting
> > memory on different places, by looking at the backtrace you used on the ovs-dev
> > mailing list.
>
> Yes. That's why I want to disable it completely on i386 until upstream comes
> up with a root-cause/fix (which they haven't so far).
>
>
> > In any case, I failed to identify the test results with the patch applied on
> > the bug. Can you tell more about them?
>
> The simple case is to just add and delete an OVS bridge repeatedly which kills
> the (i386) machine without this patch.
>
> ...Juerg

This bug might be a duplicate of LP: #1813244.

I've tried to reproduce the problem creating and deleting a bridge in a
busy loop and with LP: #1813244's fix applied the bug doesn't seem to
happen anymore.

-Andrea

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [Bionic/Cosmic/Disco/Unstable][SRU][PATCH 0/1] openvswitch: kernel oops destroying interfaces on i386 (LP #1736390)

Juerg Haefliger
In reply to this post by Juerg Haefliger
Nack'ing this patch as the issue is fixed by [1].

...Juerg

[1] https://lists.ubuntu.com/archives/kernel-team/2019-April/099883.html


On Mon, 11 Mar 2019 08:36:42 +0100
Juerg Haefliger <[hidden email]> wrote:

> The commit referenced in the patch introduced a regression on i386. Simply
> adding/deleting an OVS bridge in a loop eventually triggers a crash.
> Upstream is looking at the problem (I think) but nothing's been found yet.
> In the meantime, disable that logic on i386 to get rid of the crash. Per
> upstream, this only results in higher CPU usage and potential buffering
> issues.
>
> Juerg Haefliger (1):
>   UBUNTU: SAUCE: openvswitch: Disable eventmask support on 32-bit
>
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/conntrack.c      | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment