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

[Paolo Abeni <pabeni@redhat.com>]

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