| Message ID | 1bdeb2f3a812bca016a225d3de714427b2cd4772.1741457143.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1063ae07383c0ddc5bcce170260c143825846b03 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] Revert "openvswitch: switch to per-action label counting in conntrack" | expand |
Xin Long <lucien.xin@gmail.com> writes: > Currently, ovs_ct_set_labels() is only called for confirmed conntrack > entries (ct) within ovs_ct_commit(). However, if the conntrack entry > does not have the labels_ext extension, attempting to allocate it in > ovs_ct_get_conn_labels() for a confirmed entry triggers a warning in > nf_ct_ext_add(): > > WARN_ON(nf_ct_is_confirmed(ct)); > > This happens when the conntrack entry is created externally before OVS > increments net->ct.labels_used. The issue has become more likely since > commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > in conntrack"), which changed to use per-action label counting and > increment net->ct.labels_used when a flow with ct action is added. > > Since there’s no straightforward way to fully resolve this issue at the > moment, this reverts the commit to avoid breaking existing use cases. > > Fixes: fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack") > Reported-by: Jianbo Liu <jianbol@nvidia.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- I did a quick test using the case provided by Jianbo and I wasn't able to generate the warning. If possible, I'd like Jianbo to confirm that it works as well. Acked-by: Aaron Conole <aconole@redhat.com> > net/openvswitch/conntrack.c | 30 ++++++++++++++++++------------ > net/openvswitch/datapath.h | 3 +++ > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 3bb4810234aa..e573e9221302 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -1368,8 +1368,11 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr) > attr == OVS_KEY_ATTR_CT_MARK) > return true; > if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) && > - attr == OVS_KEY_ATTR_CT_LABELS) > - return true; > + attr == OVS_KEY_ATTR_CT_LABELS) { > + struct ovs_net *ovs_net = net_generic(net, ovs_net_id); > + > + return ovs_net->xt_label; > + } > > return false; > } > @@ -1378,7 +1381,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, > const struct sw_flow_key *key, > struct sw_flow_actions **sfa, bool log) > { > - unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; > struct ovs_conntrack_info ct_info; > const char *helper = NULL; > u16 family; > @@ -1407,12 +1409,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, > return -ENOMEM; > } > > - if (nf_connlabels_get(net, n_bits - 1)) { > - nf_ct_tmpl_free(ct_info.ct); > - OVS_NLERR(log, "Failed to set connlabel length"); > - return -EOPNOTSUPP; > - } > - > if (ct_info.timeout[0]) { > if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto, > ct_info.timeout)) > @@ -1581,7 +1577,6 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) > if (ct_info->ct) { > if (ct_info->timeout[0]) > nf_ct_destroy_timeout(ct_info->ct); > - nf_connlabels_put(nf_ct_net(ct_info->ct)); > nf_ct_tmpl_free(ct_info->ct); > } > } > @@ -2006,9 +2001,17 @@ struct genl_family dp_ct_limit_genl_family __ro_after_init = { > > int ovs_ct_init(struct net *net) > { > -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) > + unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; > struct ovs_net *ovs_net = net_generic(net, ovs_net_id); > > + if (nf_connlabels_get(net, n_bits - 1)) { > + ovs_net->xt_label = false; > + OVS_NLERR(true, "Failed to set connlabel length"); > + } else { > + ovs_net->xt_label = true; > + } > + > +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) > return ovs_ct_limit_init(net, ovs_net); > #else > return 0; > @@ -2017,9 +2020,12 @@ int ovs_ct_init(struct net *net) > > void ovs_ct_exit(struct net *net) > { > -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) > struct ovs_net *ovs_net = net_generic(net, ovs_net_id); > > +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) > ovs_ct_limit_exit(net, ovs_net); > #endif > + > + if (ovs_net->xt_label) > + nf_connlabels_put(net); > } > diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h > index 365b9bb7f546..9ca6231ea647 100644 > --- a/net/openvswitch/datapath.h > +++ b/net/openvswitch/datapath.h > @@ -160,6 +160,9 @@ struct ovs_net { > #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) > struct ovs_ct_limit_info *ct_limit_info; > #endif > + > + /* Module reference for configuring conntrack. */ > + bool xt_label; > }; > > /**
On 3/12/2025 10:25 PM, Aaron Conole wrote: > Xin Long <lucien.xin@gmail.com> writes: > >> Currently, ovs_ct_set_labels() is only called for confirmed conntrack >> entries (ct) within ovs_ct_commit(). However, if the conntrack entry >> does not have the labels_ext extension, attempting to allocate it in >> ovs_ct_get_conn_labels() for a confirmed entry triggers a warning in >> nf_ct_ext_add(): >> >> WARN_ON(nf_ct_is_confirmed(ct)); >> >> This happens when the conntrack entry is created externally before OVS >> increments net->ct.labels_used. The issue has become more likely since >> commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting >> in conntrack"), which changed to use per-action label counting and >> increment net->ct.labels_used when a flow with ct action is added. >> >> Since there’s no straightforward way to fully resolve this issue at the >> moment, this reverts the commit to avoid breaking existing use cases. >> >> Fixes: fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack") >> Reported-by: Jianbo Liu <jianbol@nvidia.com> >> Signed-off-by: Xin Long <lucien.xin@gmail.com> >> --- > > I did a quick test using the case provided by Jianbo and I wasn't able > to generate the warning. If possible, I'd like Jianbo to confirm that > it works as well. Yes, it works. The warning is gone after applying this revert. Thanks! > > Acked-by: Aaron Conole <aconole@redhat.com> > >> net/openvswitch/conntrack.c | 30 ++++++++++++++++++------------ >> net/openvswitch/datapath.h | 3 +++ >> 2 files changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index 3bb4810234aa..e573e9221302 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -1368,8 +1368,11 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr) >> attr == OVS_KEY_ATTR_CT_MARK) >> return true; >> if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) && >> - attr == OVS_KEY_ATTR_CT_LABELS) >> - return true; >> + attr == OVS_KEY_ATTR_CT_LABELS) { >> + struct ovs_net *ovs_net = net_generic(net, ovs_net_id); >> + >> + return ovs_net->xt_label; >> + } >> >> return false; >> } >> @@ -1378,7 +1381,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, >> const struct sw_flow_key *key, >> struct sw_flow_actions **sfa, bool log) >> { >> - unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; >> struct ovs_conntrack_info ct_info; >> const char *helper = NULL; >> u16 family; >> @@ -1407,12 +1409,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, >> return -ENOMEM; >> } >> >> - if (nf_connlabels_get(net, n_bits - 1)) { >> - nf_ct_tmpl_free(ct_info.ct); >> - OVS_NLERR(log, "Failed to set connlabel length"); >> - return -EOPNOTSUPP; >> - } >> - >> if (ct_info.timeout[0]) { >> if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto, >> ct_info.timeout)) >> @@ -1581,7 +1577,6 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) >> if (ct_info->ct) { >> if (ct_info->timeout[0]) >> nf_ct_destroy_timeout(ct_info->ct); >> - nf_connlabels_put(nf_ct_net(ct_info->ct)); >> nf_ct_tmpl_free(ct_info->ct); >> } >> } >> @@ -2006,9 +2001,17 @@ struct genl_family dp_ct_limit_genl_family __ro_after_init = { >> >> int ovs_ct_init(struct net *net) >> { >> -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) >> + unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; >> struct ovs_net *ovs_net = net_generic(net, ovs_net_id); >> >> + if (nf_connlabels_get(net, n_bits - 1)) { >> + ovs_net->xt_label = false; >> + OVS_NLERR(true, "Failed to set connlabel length"); >> + } else { >> + ovs_net->xt_label = true; >> + } >> + >> +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) >> return ovs_ct_limit_init(net, ovs_net); >> #else >> return 0; >> @@ -2017,9 +2020,12 @@ int ovs_ct_init(struct net *net) >> >> void ovs_ct_exit(struct net *net) >> { >> -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) >> struct ovs_net *ovs_net = net_generic(net, ovs_net_id); >> >> +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) >> ovs_ct_limit_exit(net, ovs_net); >> #endif >> + >> + if (ovs_net->xt_label) >> + nf_connlabels_put(net); >> } >> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h >> index 365b9bb7f546..9ca6231ea647 100644 >> --- a/net/openvswitch/datapath.h >> +++ b/net/openvswitch/datapath.h >> @@ -160,6 +160,9 @@ struct ovs_net { >> #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) >> struct ovs_ct_limit_info *ct_limit_info; >> #endif >> + >> + /* Module reference for configuring conntrack. */ >> + bool xt_label; >> }; >> >> /** >
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sat, 8 Mar 2025 13:05:43 -0500 you wrote: > Currently, ovs_ct_set_labels() is only called for confirmed conntrack > entries (ct) within ovs_ct_commit(). However, if the conntrack entry > does not have the labels_ext extension, attempting to allocate it in > ovs_ct_get_conn_labels() for a confirmed entry triggers a warning in > nf_ct_ext_add(): > > WARN_ON(nf_ct_is_confirmed(ct)); > > [...] Here is the summary with links: - [net] Revert "openvswitch: switch to per-action label counting in conntrack" https://git.kernel.org/netdev/net/c/1063ae07383c You are awesome, thank you!
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index 3bb4810234aa..e573e9221302 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -1368,8 +1368,11 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr) attr == OVS_KEY_ATTR_CT_MARK) return true; if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) && - attr == OVS_KEY_ATTR_CT_LABELS) - return true; + attr == OVS_KEY_ATTR_CT_LABELS) { + struct ovs_net *ovs_net = net_generic(net, ovs_net_id); + + return ovs_net->xt_label; + } return false; } @@ -1378,7 +1381,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, const struct sw_flow_key *key, struct sw_flow_actions **sfa, bool log) { - unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; struct ovs_conntrack_info ct_info; const char *helper = NULL; u16 family; @@ -1407,12 +1409,6 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, return -ENOMEM; } - if (nf_connlabels_get(net, n_bits - 1)) { - nf_ct_tmpl_free(ct_info.ct); - OVS_NLERR(log, "Failed to set connlabel length"); - return -EOPNOTSUPP; - } - if (ct_info.timeout[0]) { if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto, ct_info.timeout)) @@ -1581,7 +1577,6 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) if (ct_info->ct) { if (ct_info->timeout[0]) nf_ct_destroy_timeout(ct_info->ct); - nf_connlabels_put(nf_ct_net(ct_info->ct)); nf_ct_tmpl_free(ct_info->ct); } } @@ -2006,9 +2001,17 @@ struct genl_family dp_ct_limit_genl_family __ro_after_init = { int ovs_ct_init(struct net *net) { -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) + unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE; struct ovs_net *ovs_net = net_generic(net, ovs_net_id); + if (nf_connlabels_get(net, n_bits - 1)) { + ovs_net->xt_label = false; + OVS_NLERR(true, "Failed to set connlabel length"); + } else { + ovs_net->xt_label = true; + } + +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) return ovs_ct_limit_init(net, ovs_net); #else return 0; @@ -2017,9 +2020,12 @@ int ovs_ct_init(struct net *net) void ovs_ct_exit(struct net *net) { -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) struct ovs_net *ovs_net = net_generic(net, ovs_net_id); +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) ovs_ct_limit_exit(net, ovs_net); #endif + + if (ovs_net->xt_label) + nf_connlabels_put(net); } diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h index 365b9bb7f546..9ca6231ea647 100644 --- a/net/openvswitch/datapath.h +++ b/net/openvswitch/datapath.h @@ -160,6 +160,9 @@ struct ovs_net { #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) struct ovs_ct_limit_info *ct_limit_info; #endif + + /* Module reference for configuring conntrack. */ + bool xt_label; }; /**
Currently, ovs_ct_set_labels() is only called for confirmed conntrack entries (ct) within ovs_ct_commit(). However, if the conntrack entry does not have the labels_ext extension, attempting to allocate it in ovs_ct_get_conn_labels() for a confirmed entry triggers a warning in nf_ct_ext_add(): WARN_ON(nf_ct_is_confirmed(ct)); This happens when the conntrack entry is created externally before OVS increments net->ct.labels_used. The issue has become more likely since commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack"), which changed to use per-action label counting and increment net->ct.labels_used when a flow with ct action is added. Since there’s no straightforward way to fully resolve this issue at the moment, this reverts the commit to avoid breaking existing use cases. Fixes: fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack") Reported-by: Jianbo Liu <jianbol@nvidia.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/openvswitch/conntrack.c | 30 ++++++++++++++++++------------ net/openvswitch/datapath.h | 3 +++ 2 files changed, 21 insertions(+), 12 deletions(-)