| Message ID | b7c05496f8ead33582eb561b55d3e2fcf25bcf36.1741108507.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] openvswitch: avoid allocating labels_ext in ovs_ct_set_labels | 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 > increases net->ct.labels_used. The issue has become more likely since > commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > in conntrack"), which switched to per-action label counting. > > To prevent this warning, this patch modifies ovs_ct_set_labels() to > call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where > it allocates the labels_ext if it does not exist, aligning its > behavior with tcf_ct_act_set_labels(). > > 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> > --- Just a nit, but after this change, the only user of the ovs_ct_get_conn_labels function is in the init path. I think it might make sense to also rename it to something like 'ovs_ct_init_labels_ext'. Then hopefully it would be clear not to use it outside of the initialization path. > net/openvswitch/conntrack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 3bb4810234aa..f13fbab4c942 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > struct nf_conn_labels *cl; > int err; > > - cl = ovs_ct_get_conn_labels(ct); > + cl = nf_ct_labels_find(ct); > if (!cl) > return -ENOSPC;
On 3/5/2025 1:15 AM, Xin Long 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)); > > This happens when the conntrack entry is created externally before OVS > increases net->ct.labels_used. The issue has become more likely since > commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > in conntrack"), which switched to per-action label counting. > > To prevent this warning, this patch modifies ovs_ct_set_labels() to > call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where > it allocates the labels_ext if it does not exist, aligning its > behavior with tcf_ct_act_set_labels(). > > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 3bb4810234aa..f13fbab4c942 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > struct nf_conn_labels *cl; > int err; > > - cl = ovs_ct_get_conn_labels(ct); > + cl = nf_ct_labels_find(ct); I don't think it's correct fix. The label is not added and packets can't pass the next rule to match ct_label. I tested it and used the configuration posted before, ping can't work. > if (!cl) > return -ENOSPC; >
On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <jianbol@nvidia.com> wrote: > > > > On 3/5/2025 1:15 AM, Xin Long 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)); > > > > This happens when the conntrack entry is created externally before OVS > > increases net->ct.labels_used. The issue has become more likely since > > commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > > in conntrack"), which switched to per-action label counting. > > > > To prevent this warning, this patch modifies ovs_ct_set_labels() to > > call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where > > it allocates the labels_ext if it does not exist, aligning its > > behavior with tcf_ct_act_set_labels(). > > > > 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 | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > > index 3bb4810234aa..f13fbab4c942 100644 > > --- a/net/openvswitch/conntrack.c > > +++ b/net/openvswitch/conntrack.c > > @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > > struct nf_conn_labels *cl; > > int err; > > > > - cl = ovs_ct_get_conn_labels(ct); > > + cl = nf_ct_labels_find(ct); > > I don't think it's correct fix. The label is not added and packets can't > pass the next rule to match ct_label. > That's expected, external ct may not work in the flow with extensions. Again, "openvswitch: switch to per-action label counting in conntrack" only makes it easier to be triggered. > I tested it and used the configuration posted before, ping can't work. I've provided the 'workaround' with the ct zone for this in the other email. I would also like to see the maintainer's option about this. Thanks.
On Tue, Mar 4, 2025 at 8:00 PM Aaron Conole <aconole@redhat.com> 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 > > increases net->ct.labels_used. The issue has become more likely since > > commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > > in conntrack"), which switched to per-action label counting. > > > > To prevent this warning, this patch modifies ovs_ct_set_labels() to > > call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where > > it allocates the labels_ext if it does not exist, aligning its > > behavior with tcf_ct_act_set_labels(). > > > > 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> > > --- > > Just a nit, but after this change, the only user of the > ovs_ct_get_conn_labels function is in the init path. I think it might > make sense to also rename it to something like 'ovs_ct_init_labels_ext'. > Then hopefully it would be clear not to use it outside of the > initialization path. Right. If you're okay with it, I'd like to delete this function and directly use the following code in ovs_ct_init_labels(): cl = nf_ct_labels_find(ct); if (!cl) { nf_ct_labels_ext_add(ct); cl = nf_ct_labels_find(ct); } since the function was originally introduced for only those two places. Thanks. > > > net/openvswitch/conntrack.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > > index 3bb4810234aa..f13fbab4c942 100644 > > --- a/net/openvswitch/conntrack.c > > +++ b/net/openvswitch/conntrack.c > > @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > > struct nf_conn_labels *cl; > > int err; > > > > - cl = ovs_ct_get_conn_labels(ct); > > + cl = nf_ct_labels_find(ct); > > if (!cl) > > return -ENOSPC; >
On 3/5/25 15:59, Xin Long wrote: > On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <jianbol@nvidia.com> wrote: >> >> >> >> On 3/5/2025 1:15 AM, Xin Long 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)); >>> >>> This happens when the conntrack entry is created externally before OVS >>> increases net->ct.labels_used. The issue has become more likely since >>> commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting >>> in conntrack"), which switched to per-action label counting. >>> >>> To prevent this warning, this patch modifies ovs_ct_set_labels() to >>> call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where >>> it allocates the labels_ext if it does not exist, aligning its >>> behavior with tcf_ct_act_set_labels(). >>> >>> 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 | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>> index 3bb4810234aa..f13fbab4c942 100644 >>> --- a/net/openvswitch/conntrack.c >>> +++ b/net/openvswitch/conntrack.c >>> @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, >>> struct nf_conn_labels *cl; >>> int err; >>> >>> - cl = ovs_ct_get_conn_labels(ct); >>> + cl = nf_ct_labels_find(ct); >> >> I don't think it's correct fix. The label is not added and packets can't >> pass the next rule to match ct_label. >> > That's expected, external ct may not work in the flow with extensions. > Again, "openvswitch: switch to per-action label counting in conntrack" > only makes it easier to be triggered. > >> I tested it and used the configuration posted before, ping can't work. > I've provided the 'workaround' with the ct zone for this in the other email. I think, the test provided in the other thread is reasonable and logically correct. The link for the test: https://lore.kernel.org/all/2ee4d016-5e57-4d86-9dca-e4685cb183bb@nvidia.com/ We should be able to match on the label committed in the original direction. The workaround doesn't really cover the use case, because the labels cover a much larger scope that zones and it may be not possible to use zones instead of labels. Also, the ct entry obtained in the test is not from outside, AFAICT, it is created inside the OVS pipeline and labels are also created inside the OVS datapath, so it should work. On a side note, the fact that it's allowed to modify labels for committed connections, but it's not allowed to set one when there is none, seems like an inconsistent behavior. Maybe that should be fixed and the warning removed? Best regards, Ilya Maximets. > > I would also like to see the maintainer's option about this. > > Thanks.
On Wed, Mar 5, 2025 at 10:35 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 3/5/25 15:59, Xin Long wrote: > > On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <jianbol@nvidia.com> wrote: > >> > >> > >> > >> On 3/5/2025 1:15 AM, Xin Long 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)); > >>> > >>> This happens when the conntrack entry is created externally before OVS > >>> increases net->ct.labels_used. The issue has become more likely since > >>> commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting > >>> in conntrack"), which switched to per-action label counting. > >>> > >>> To prevent this warning, this patch modifies ovs_ct_set_labels() to > >>> call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where > >>> it allocates the labels_ext if it does not exist, aligning its > >>> behavior with tcf_ct_act_set_labels(). > >>> > >>> 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 | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > >>> index 3bb4810234aa..f13fbab4c942 100644 > >>> --- a/net/openvswitch/conntrack.c > >>> +++ b/net/openvswitch/conntrack.c > >>> @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > >>> struct nf_conn_labels *cl; > >>> int err; > >>> > >>> - cl = ovs_ct_get_conn_labels(ct); > >>> + cl = nf_ct_labels_find(ct); > >> > >> I don't think it's correct fix. The label is not added and packets can't > >> pass the next rule to match ct_label. > >> > > That's expected, external ct may not work in the flow with extensions. > > Again, "openvswitch: switch to per-action label counting in conntrack" > > only makes it easier to be triggered. > > > >> I tested it and used the configuration posted before, ping can't work. > > I've provided the 'workaround' with the ct zone for this in the other email. > > I think, the test provided in the other thread is reasonable and logically > correct. The link for the test: > https://lore.kernel.org/all/2ee4d016-5e57-4d86-9dca-e4685cb183bb@nvidia.com/ > > We should be able to match on the label committed in the original direction. > The workaround doesn't really cover the use case, because the labels cover > a much larger scope that zones and it may be not possible to use zones instead > of labels. Also, the ct entry obtained in the test is not from outside, AFAICT, > it is created inside the OVS pipeline and labels are also created inside the > OVS datapath, so it should work. All ct entries created by action=ct() will have label exts allocated. This is because any flow with ct inserted in the kernel increases net->ct.labels_used via nf_connlabels_get(), which ensures that the new ct entry has label exts allocated during its creation. Therefore, I believed this ct entry was created outside of OVS. But I might not be aware of other scenarios where OVS itself might create ct entries. > > On a side note, the fact that it's allowed to modify labels for committed > connections, but it's not allowed to set one when there is none, seems like > an inconsistent behavior. Maybe that should be fixed and the warning removed? I need to check with Netfilter developers regarding this. If I can't find a solution this week, I will revert "openvswitch: switch to per-action label counting in conntrack" as requested by Jianbo. Thanks. > > Best regards, Ilya Maximets. > > > > > I would also like to see the maintainer's option about this. > > > > Thanks. >
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index 3bb4810234aa..f13fbab4c942 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, struct nf_conn_labels *cl; int err; - cl = ovs_ct_get_conn_labels(ct); + cl = nf_ct_labels_find(ct); if (!cl) return -ENOSPC;
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 increases net->ct.labels_used. The issue has become more likely since commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack"), which switched to per-action label counting. To prevent this warning, this patch modifies ovs_ct_set_labels() to call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where it allocates the labels_ext if it does not exist, aligning its behavior with tcf_ct_act_set_labels(). 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)