| Message ID | 1566505070-38748-1-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] openvswitch: Fix conntrack cache with timeout | expand |
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?
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
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.
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.
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) {
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(+)