[PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Xin Long]

A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
applies on a confirmed connection:

  WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
  RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
  Call Trace:
   <TASK>
   nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
   __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
   __ovs_ct_lookup+0x72e/0x780 [openvswitch]
   ovs_ct_execute+0x1d8/0x920 [openvswitch]
   do_execute_actions+0x4e6/0xb60 [openvswitch]
   ovs_execute_actions+0x60/0x140 [openvswitch]
   ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
   genl_family_rcv_msg_doit.isra.15+0x113/0x150
   genl_rcv_msg+0xef/0x1f0

which can be reproduced with these OVS flows:

  table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
  actions=ct(commit, table=1)
  table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
  actions=ct(commit, alg=ftp),normal

The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
attaching helper in later commit") where it somehow removed the check
of nf_ct_is_confirmed before asigning the helper. This patch is to fix
it by bringing it back.

Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/openvswitch/conntrack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 4e70df91d0f2..6862475f0f88 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1015,7 +1015,8 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		 * connections which we will commit, we may need to attach
 		 * the helper here.
 		 */
-		if (info->commit && info->helper && !nfct_help(ct)) {
+		if (!nf_ct_is_confirmed(ct) && info->commit &&
+		    info->helper && !nfct_help(ct)) {
 			int err = __nf_ct_try_assign_helper(ct, info->ct,
 							    GFP_ATOMIC);
 			if (err)
-- 
2.31.1

Re: [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Paolo Abeni]

Hello,

On Thu, 2022-10-06 at 15:45 -0400, Xin Long wrote:
> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> applies on a confirmed connection:
> 
>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>   Call Trace:
>    <TASK>
>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>    ovs_execute_actions+0x60/0x140 [openvswitch]
>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>    genl_rcv_msg+0xef/0x1f0
> 
> which can be reproduced with these OVS flows:
> 
>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
>   actions=ct(commit, table=1)
>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
>   actions=ct(commit, alg=ftp),normal
> 
> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
> attaching helper in later commit") where it somehow removed the check
> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
> it by bringing it back.
> 
> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/openvswitch/conntrack.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 4e70df91d0f2..6862475f0f88 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1015,7 +1015,8 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  		 * connections which we will commit, we may need to attach
>  		 * the helper here.
>  		 */
> -		if (info->commit && info->helper && !nfct_help(ct)) {
> +		if (!nf_ct_is_confirmed(ct) && info->commit &&
> +		    info->helper && !nfct_help(ct)) {
>  			int err = __nf_ct_try_assign_helper(ct, info->ct,
>  							    GFP_ATOMIC);
>  			if (err)

The patch LGTM, but it would be great if someone from the OVS side
could confirm this does not break existing use-case.

Thanks!

Paolo

Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Aaron Conole]

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

> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> applies on a confirmed connection:
>
>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>   Call Trace:
>    <TASK>
>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>    ovs_execute_actions+0x60/0x140 [openvswitch]
>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>    genl_rcv_msg+0xef/0x1f0
>
> which can be reproduced with these OVS flows:
>
>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
>   actions=ct(commit, table=1)
>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
>   actions=ct(commit, alg=ftp),normal
>
> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
> attaching helper in later commit") where it somehow removed the check
> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
> it by bringing it back.
>
> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---

Hi Xin,

Looking at the original commit, I think this will read like a revert.  I
am doing some testing now, but I think we need input from Yi-Hung to
find out what the use case is that the original fixed.

-Aaron

Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Aaron Conole]

Aaron Conole <aconole@redhat.com> writes:

> Xin Long <lucien.xin@gmail.com> writes:
>
>> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
>> applies on a confirmed connection:
>>
>>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>>   Call Trace:
>>    <TASK>
>>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>>    ovs_execute_actions+0x60/0x140 [openvswitch]
>>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>>    genl_rcv_msg+0xef/0x1f0
>>
>> which can be reproduced with these OVS flows:
>>
>>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
>>   actions=ct(commit, table=1)
>>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
>>   actions=ct(commit, alg=ftp),normal
>>
>> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
>> attaching helper in later commit") where it somehow removed the check
>> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
>> it by bringing it back.
>>
>> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
>> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>
> Hi Xin,
>
> Looking at the original commit, I think this will read like a revert.  I
> am doing some testing now, but I think we need input from Yi-Hung to
> find out what the use case is that the original fixed.

I'm also not able to reproduce the WARN_ON.  My env:

kernel: 4c86114194e6 ("Merge tag 'iomap-6.1-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux")

Using current upstream OVS
I used your flows (adjusting the port names):

 cookie=0x0, duration=246.240s, table=0, n_packets=17, n_bytes=1130, ct_state=-trk,tcp,in_port=v0,tp_dst=2121 actions=ct(commit,table=1)
 cookie=0x0, duration=246.240s, table=1, n_packets=1, n_bytes=74, ct_state=+new+trk,tcp,in_port=v0,tp_dst=2121 actions=ct(commit,alg=ftp),NORMAL

and ran:

$ ip netns exec server python3 -m pyftpdlib -i 172.31.110.20 &
$ ip netns exec client curl ftp://172.31.110.20:2121

but no WARN_ON message got triggered.  Are there additional flows you
used that I am missing, or perhaps this should be on a different kernel
commit?

> -Aaron

Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Xin Long]

On Tue, Oct 11, 2022 at 10:06 AM Aaron Conole <aconole@redhat.com> wrote:
>
> Aaron Conole <aconole@redhat.com> writes:
>
> > Xin Long <lucien.xin@gmail.com> writes:
> >
> >> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> >> applies on a confirmed connection:
> >>
> >>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
> >>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
> >>   Call Trace:
> >>    <TASK>
> >>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
> >>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
> >>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
> >>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
> >>    do_execute_actions+0x4e6/0xb60 [openvswitch]
> >>    ovs_execute_actions+0x60/0x140 [openvswitch]
> >>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
> >>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
> >>    genl_rcv_msg+0xef/0x1f0
> >>
> >> which can be reproduced with these OVS flows:
> >>
> >>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
> >>   actions=ct(commit, table=1)
> >>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
> >>   actions=ct(commit, alg=ftp),normal
> >>
> >> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
> >> attaching helper in later commit") where it somehow removed the check
> >> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
> >> it by bringing it back.
> >>
> >> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
> >> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >> ---
> >
> > Hi Xin,
> >
> > Looking at the original commit, I think this will read like a revert.  I
> > am doing some testing now, but I think we need input from Yi-Hung to
> > find out what the use case is that the original fixed.
>
> I'm also not able to reproduce the WARN_ON.  My env:
>
> kernel: 4c86114194e6 ("Merge tag 'iomap-6.1-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux")
>
> Using current upstream OVS
> I used your flows (adjusting the port names):
>
>  cookie=0x0, duration=246.240s, table=0, n_packets=17, n_bytes=1130, ct_state=-trk,tcp,in_port=v0,tp_dst=2121 actions=ct(commit,table=1)
>  cookie=0x0, duration=246.240s, table=1, n_packets=1, n_bytes=74, ct_state=+new+trk,tcp,in_port=v0,tp_dst=2121 actions=ct(commit,alg=ftp),NORMAL
>
> and ran:
>
> $ ip netns exec server python3 -m pyftpdlib -i 172.31.110.20 &
> $ ip netns exec client curl ftp://172.31.110.20:2121
>
> but no WARN_ON message got triggered.  Are there additional flows you
> used that I am missing, or perhaps this should be on a different kernel
> commit?
>
> > -Aaron
>
Hi, Aaron, thanks for looking at this.

Here is a script to reproduce it, you can try, and let me know if it's
still not reproduced
(it's also upstream ovs, for the first 2 lines, you may adjust on your env.)

export PATH=$PATH:/usr/local/share/openvswitch/scripts
ovs-ctl restart #systemctl restart openvswitch

ip net add ns0
ip net add ns1
ip link add veth0 type veth peer name veth1
ip link add veth3 type veth peer name veth2
ip link set veth0 netns ns0
ip link set veth3 netns ns1
ip net exec ns0 ip addr add  7.7.7.1/24 dev veth0
mac1=`ip net exec ns1 cat /sys/class/net/veth3/address`
mac2=`ip net exec ns0 cat /sys/class/net/veth0/address`
ip net exec ns0 ip neigh add 7.7.16.2 dev veth0 lladdr $mac1
ip net exec ns1 ip addr add  7.7.16.2/24 dev veth3
ip net exec ns1 ip neigh add 7.7.16.1 dev veth3 lladdr $mac2
ip net exec ns0 ip link set veth0 up
ip net exec ns1 ip link set veth3 up
ip net exec ns0 ip route add  7.7.16.2 dev veth0

sleep 0.5
ovs-vsctl set Open_vSwitch . other_config:hw-offload=false
ovs-vsctl set Open_vSwitch . other_config:tc-policy=skip_hw
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs veth1
ovs-vsctl add-port br-ovs veth2
ip link set br-ovs up
ip link set veth1 up
ip link set veth2 up

ovs-ofctl del-flows br-ovs
ovs-ofctl add-flow br-ovs arp,actions=normal
ovs-ofctl add-flow br-ovs icmp,actions=normal
ovs-ofctl add-flow br-ovs "table=0,
in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk actions=ct(commit,
table=1)"
#ovs-ofctl add-flow br-ovs "table=0,
in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk actions=ct(table=1, nat)"
ovs-ofctl add-flow br-ovs "table=0, in_port=veth2,tcp,ct_state=-trk
actions=ct(table=1, nat)"
ovs-ofctl add-flow br-ovs "table=0, in_port=veth1,tcp,ct_state=-trk
actions=ct(table=0, nat)"

ovs-ofctl add-flow br-ovs "table=0,
in_port=veth1,tcp,ct_state=+trk+rel actions=ct(commit, nat),normal"
ovs-ofctl add-flow br-ovs "table=0,
in_port=veth1,tcp,ct_state=+trk+est actions=veth2"

ovs-ofctl add-flow br-ovs "table=1,
in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new actions=ct(commit)"
ovs-ofctl add-flow br-ovs "table=1,
in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new actions=ct(commit,
alg=ftp),normal"

ovs-ofctl add-flow br-ovs "table=1,
in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new actions=ct(commit,
nat(src=7.7.16.1), alg=ftp),normal"
ovs-ofctl add-flow br-ovs "table=1,
in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+est actions=veth2"
ovs-ofctl add-flow br-ovs "table=1,
in_port=veth2,tcp,ct_state=+trk+est actions=veth1"
ovs-ofctl dump-flows br-ovs --color
conntrack -F

ip netns exec ns1 echo "test" > a
ip netns exec ns1 python3 -m pyftpdlib -p 2121 -D &
sleep 2
ip netns exec ns0 wget ftp://anonymous@7.7.16.2:2121/a

Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[Aaron Conole]

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

> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> applies on a confirmed connection:
>
>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>   Call Trace:
>    <TASK>
>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>    ovs_execute_actions+0x60/0x140 [openvswitch]
>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>    genl_rcv_msg+0xef/0x1f0
>
> which can be reproduced with these OVS flows:
>
>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
>   actions=ct(commit, table=1)
>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
>   actions=ct(commit, alg=ftp),normal
>
> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
> attaching helper in later commit") where it somehow removed the check
> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
> it by bringing it back.
>
> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---

Thanks for all the help, I was able to reproduce the warning.  Looking
at it, I'm not sure why the check for confirmed was removed.

I did some additional testing with common scenarios, some testing with
bare DP flows, and I think it is good to apply.

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

Re: [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  6 Oct 2022 15:45:02 -0400 you wrote:
> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> applies on a confirmed connection:
> 
>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>   Call Trace:
>    <TASK>
>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>    ovs_execute_actions+0x60/0x140 [openvswitch]
>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>    genl_rcv_msg+0xef/0x1f0
> 
> [...]

Here is the summary with links:
  - [net] openvswitch: add nf_ct_is_confirmed check before assigning the helper
    https://git.kernel.org/netdev/net/c/3c1860543fcc

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