[PATCH net v2] openvswitch: Fix conntrack cache with timeout

[PATCH net v2] openvswitch: Fix conntrack cache with timeout

[Yi-Hung Wei]

This patch addresses a conntrack cache issue with timeout policy.
Currently, we do not check if the timeout extension is set properly in the
cached conntrack entry.  Thus, after packet recirculate from conntrack
action, the timeout policy is not applied properly.  This patch fixes the
aforementioned issue.

Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
v1->v2: Fix rcu dereference issue reported by kbuild test robot.
---
 net/openvswitch/conntrack.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 848c6eb55064..4d7896135e73 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -67,6 +67,7 @@ struct ovs_conntrack_info {
 	struct md_mark mark;
 	struct md_labels labels;
 	char timeout[CTNL_TIMEOUT_NAME_MAX];
+	struct nf_ct_timeout *nf_ct_timeout;
 #if IS_ENABLED(CONFIG_NF_NAT)
 	struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. */
 #endif
@@ -697,6 +698,14 @@ static bool skb_nfct_cached(struct net *net,
 		if (help && rcu_access_pointer(help->helper) != info->helper)
 			return false;
 	}
+	if (info->nf_ct_timeout) {
+		struct nf_conn_timeout *timeout_ext;
+
+		timeout_ext = nf_ct_timeout_find(ct);
+		if (!timeout_ext || info->nf_ct_timeout !=
+		    rcu_dereference(timeout_ext->timeout))
+			return false;
+	}
 	/* Force conntrack entry direction to the current packet? */
 	if (info->force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
 		/* Delete the conntrack entry if confirmed, else just release
@@ -1657,6 +1666,10 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 				      ct_info.timeout))
 			pr_info_ratelimited("Failed to associated timeout "
 					    "policy `%s'\n", ct_info.timeout);
+		else
+			ct_info.nf_ct_timeout = rcu_dereference(
+				nf_ct_timeout_find(ct_info.ct)->timeout);
+
 	}
 
 	if (helper) {
-- 
2.7.4

Re: [PATCH net v2] openvswitch: Fix conntrack cache with timeout

[Pravin Shelar]

On Thu, Aug 22, 2019 at 1:28 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
>
> This patch addresses a conntrack cache issue with timeout policy.
> Currently, we do not check if the timeout extension is set properly in the
> cached conntrack entry.  Thus, after packet recirculate from conntrack
> action, the timeout policy is not applied properly.  This patch fixes the
> aforementioned issue.
>
> Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
> v1->v2: Fix rcu dereference issue reported by kbuild test robot.
> ---
>  net/openvswitch/conntrack.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 848c6eb55064..4d7896135e73 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -67,6 +67,7 @@ struct ovs_conntrack_info {
>         struct md_mark mark;
>         struct md_labels labels;
>         char timeout[CTNL_TIMEOUT_NAME_MAX];
> +       struct nf_ct_timeout *nf_ct_timeout;
>  #if IS_ENABLED(CONFIG_NF_NAT)
>         struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. */
>  #endif
> @@ -697,6 +698,14 @@ static bool skb_nfct_cached(struct net *net,
>                 if (help && rcu_access_pointer(help->helper) != info->helper)
>                         return false;
>         }
> +       if (info->nf_ct_timeout) {
> +               struct nf_conn_timeout *timeout_ext;
> +
> +               timeout_ext = nf_ct_timeout_find(ct);
> +               if (!timeout_ext || info->nf_ct_timeout !=
> +                   rcu_dereference(timeout_ext->timeout))
> +                       return false;
> +       }
>         /* Force conntrack entry direction to the current packet? */
>         if (info->force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
>                 /* Delete the conntrack entry if confirmed, else just release
> @@ -1657,6 +1666,10 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>                                       ct_info.timeout))
>                         pr_info_ratelimited("Failed to associated timeout "
>                                             "policy `%s'\n", ct_info.timeout);
> +               else
> +                       ct_info.nf_ct_timeout = rcu_dereference(
> +                               nf_ct_timeout_find(ct_info.ct)->timeout);
Is this dereference safe from NULL pointer?

Re: [PATCH net v2] openvswitch: Fix conntrack cache with timeout

[Yi-Hung Wei]

On Thu, Aug 22, 2019 at 11:51 PM Pravin Shelar <pshelar@ovn.org> wrote:
>
> On Thu, Aug 22, 2019 at 1:28 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> >
> > This patch addresses a conntrack cache issue with timeout policy.
> > Currently, we do not check if the timeout extension is set properly in the
> > cached conntrack entry.  Thus, after packet recirculate from conntrack
> > action, the timeout policy is not applied properly.  This patch fixes the
> > aforementioned issue.
> >
> > Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> > ---
> > v1->v2: Fix rcu dereference issue reported by kbuild test robot.
> > ---
> >  net/openvswitch/conntrack.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> > index 848c6eb55064..4d7896135e73 100644
> > --- a/net/openvswitch/conntrack.c
> > +++ b/net/openvswitch/conntrack.c
> > @@ -1657,6 +1666,10 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
> >                                       ct_info.timeout))
> >                         pr_info_ratelimited("Failed to associated timeout "
> >                                             "policy `%s'\n", ct_info.timeout);
> > +               else
> > +                       ct_info.nf_ct_timeout = rcu_dereference(
> > +                               nf_ct_timeout_find(ct_info.ct)->timeout);
> Is this dereference safe from NULL pointer?

Hi Pravin,

Thanks for your review.  I am not sure if
nf_ct_timeout_find(ct_info.ct) will return NULL in this case.

We only run into this statement when ct_info.timeout[0] is set, and it
is only set in parse_ct() when CONFIG_NF_CONNTRACK_TIMEOUT is
configured.  Also, in this else condition the timeout extension is
supposed to be set properly by nf_ct_set_timeout().

Am I missing something?

Thanks,

-Yi-Hung

Re: [PATCH net v2] openvswitch: Fix conntrack cache with timeout

[Pravin Shelar]

On Fri, Aug 23, 2019 at 9:40 AM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
>
> On Thu, Aug 22, 2019 at 11:51 PM Pravin Shelar <pshelar@ovn.org> wrote:
> >
> > On Thu, Aug 22, 2019 at 1:28 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> > >
> > > This patch addresses a conntrack cache issue with timeout policy.
> > > Currently, we do not check if the timeout extension is set properly in the
> > > cached conntrack entry.  Thus, after packet recirculate from conntrack
> > > action, the timeout policy is not applied properly.  This patch fixes the
> > > aforementioned issue.
> > >
> > > Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> > > ---
> > > v1->v2: Fix rcu dereference issue reported by kbuild test robot.
> > > ---
> > >  net/openvswitch/conntrack.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> > > index 848c6eb55064..4d7896135e73 100644
> > > --- a/net/openvswitch/conntrack.c
> > > +++ b/net/openvswitch/conntrack.c
> > > @@ -1657,6 +1666,10 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
> > >                                       ct_info.timeout))
> > >                         pr_info_ratelimited("Failed to associated timeout "
> > >                                             "policy `%s'\n", ct_info.timeout);
> > > +               else
> > > +                       ct_info.nf_ct_timeout = rcu_dereference(
> > > +                               nf_ct_timeout_find(ct_info.ct)->timeout);
> > Is this dereference safe from NULL pointer?
>
> Hi Pravin,
>
> Thanks for your review.  I am not sure if
> nf_ct_timeout_find(ct_info.ct) will return NULL in this case.
>
> We only run into this statement when ct_info.timeout[0] is set, and it
> is only set in parse_ct() when CONFIG_NF_CONNTRACK_TIMEOUT is
> configured.  Also, in this else condition the timeout extension is
> supposed to be set properly by nf_ct_set_timeout().
>

Sounds good.
Acked-by: Pravin B Shelar <pshelar@ovn.org>

Thanks,
Pravin.

Re: [PATCH net v2] openvswitch: Fix conntrack cache with timeout

[David Miller]

From: Yi-Hung Wei <yihung.wei@gmail.com>
Date: Thu, 22 Aug 2019 13:17:50 -0700

> This patch addresses a conntrack cache issue with timeout policy.
> Currently, we do not check if the timeout extension is set properly in the
> cached conntrack entry.  Thus, after packet recirculate from conntrack
> action, the timeout policy is not applied properly.  This patch fixes the
> aforementioned issue.
> 
> Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

Applied and queued up for -stable, thanks.