[PATCH net] openvswitch: get related ct labels from its master if it is not confirmed

[PATCH net] openvswitch: get related ct labels from its master if it is not confirmed

[Xin Long]

Ilya found a failure in running check-kernel tests with at_groups=144
(144: conntrack - FTP SNAT orig tuple) in OVS repo. After his further
investigation, the root cause is that the labels sent to userspace
for related ct are incorrect.

The labels for unconfirmed related ct should use its master's labels.
However, the changes made in commit 8c8b73320805 ("openvswitch: set
IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
led to getting labels from this related ct.

So fix it in ovs_ct_get_labels() by changing to copy labels from its
master ct if it is a unconfirmed related ct. Note that there is no
fix needed for ct->mark, as it was already copied from its master
ct for related ct in init_conntrack().

Fixes: 8c8b73320805 ("openvswitch: set IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
Reported-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/openvswitch/conntrack.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 331730fd3580..920e802ff01e 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -167,8 +167,13 @@ static u32 ovs_ct_get_mark(const struct nf_conn *ct)
 static void ovs_ct_get_labels(const struct nf_conn *ct,
 			      struct ovs_key_ct_labels *labels)
 {
-	struct nf_conn_labels *cl = ct ? nf_ct_labels_find(ct) : NULL;
+	struct nf_conn_labels *cl = NULL;
 
+	if (ct) {
+		if (ct->master && !nf_ct_is_confirmed(ct))
+			ct = ct->master;
+		cl = nf_ct_labels_find(ct);
+	}
 	if (cl)
 		memcpy(labels, cl->bits, OVS_CT_LABELS_LEN);
 	else
-- 
2.43.0

Re: [PATCH net] openvswitch: get related ct labels from its master if it is not confirmed

[Ilya Maximets]

On 6/20/24 00:08, Xin Long wrote:
> Ilya found a failure in running check-kernel tests with at_groups=144
> (144: conntrack - FTP SNAT orig tuple) in OVS repo. After his further
> investigation, the root cause is that the labels sent to userspace
> for related ct are incorrect.
> 
> The labels for unconfirmed related ct should use its master's labels.
> However, the changes made in commit 8c8b73320805 ("openvswitch: set
> IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
> led to getting labels from this related ct.
> 
> So fix it in ovs_ct_get_labels() by changing to copy labels from its
> master ct if it is a unconfirmed related ct. Note that there is no
> fix needed for ct->mark, as it was already copied from its master
> ct for related ct in init_conntrack().
> 
> Fixes: 8c8b73320805 ("openvswitch: set IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
> Reported-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/openvswitch/conntrack.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 331730fd3580..920e802ff01e 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -167,8 +167,13 @@ static u32 ovs_ct_get_mark(const struct nf_conn *ct)
>  static void ovs_ct_get_labels(const struct nf_conn *ct,
>  			      struct ovs_key_ct_labels *labels)
>  {
> -	struct nf_conn_labels *cl = ct ? nf_ct_labels_find(ct) : NULL;
> +	struct nf_conn_labels *cl = NULL;
>  
> +	if (ct) {
> +		if (ct->master && !nf_ct_is_confirmed(ct))
> +			ct = ct->master;
> +		cl = nf_ct_labels_find(ct);
> +	}
>  	if (cl)
>  		memcpy(labels, cl->bits, OVS_CT_LABELS_LEN);
>  	else

Thanks, Xin!  LGTM.

Tested with OVS testsuite and it works fine.  Also re-checked OVN
system tests and they also work as expected.

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Tested-by: Ilya Maximets <i.maximets@ovn.org>

Re: [ovs-dev] [PATCH net] openvswitch: get related ct labels from its master if it is not confirmed

[Aaron Conole]

Xin Long <lucien.xin@gmail.com> writes:

> Ilya found a failure in running check-kernel tests with at_groups=144
> (144: conntrack - FTP SNAT orig tuple) in OVS repo. After his further
> investigation, the root cause is that the labels sent to userspace
> for related ct are incorrect.
>
> The labels for unconfirmed related ct should use its master's labels.
> However, the changes made in commit 8c8b73320805 ("openvswitch: set
> IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
> led to getting labels from this related ct.
>
> So fix it in ovs_ct_get_labels() by changing to copy labels from its
> master ct if it is a unconfirmed related ct. Note that there is no
> fix needed for ct->mark, as it was already copied from its master
> ct for related ct in init_conntrack().
>
> Fixes: 8c8b73320805 ("openvswitch: set IPS_CONFIRMED in tmpl status
> only when commit is set in conntrack")
> Reported-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net] openvswitch: get related ct labels from its master if it is not confirmed

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 19 Jun 2024 18:08:56 -0400 you wrote:
> Ilya found a failure in running check-kernel tests with at_groups=144
> (144: conntrack - FTP SNAT orig tuple) in OVS repo. After his further
> investigation, the root cause is that the labels sent to userspace
> for related ct are incorrect.
> 
> The labels for unconfirmed related ct should use its master's labels.
> However, the changes made in commit 8c8b73320805 ("openvswitch: set
> IPS_CONFIRMED in tmpl status only when commit is set in conntrack")
> led to getting labels from this related ct.
> 
> [...]

Here is the summary with links:
  - [net] openvswitch: get related ct labels from its master if it is not confirmed
    https://git.kernel.org/netdev/net/c/a23ac973f67f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html