Re: [PATCH nf] sched: act_ct: additional checks for outdated flows

[Paul Blakey <paulb@nvidia.com>]

On 24/10/2023 23:02, Pablo Neira Ayuso wrote:
> From: Vlad Buslov <vladbu@nvidia.com>
> 
> Current nf_flow_is_outdated() implementation considers any flow table flow
> which state diverged from its underlying CT connection status for teardown
> which can be problematic in the following cases:
> 
> - Flow has never been offloaded to hardware in the first place either
> because flow table has hardware offload disabled (flag
> NF_FLOWTABLE_HW_OFFLOAD is not set) or because it is still pending on 'add'
> workqueue to be offloaded for the first time. The former is incorrect, the
> later generates excessive deletions and additions of flows.
> 
> - Flow is already pending to be updated on the workqueue. Tearing down such
> flows will also generate excessive removals from the flow table, especially
> on highly loaded system where the latency to re-offload a flow via 'add'
> workqueue can be quite high.
> 
> When considering a flow for teardown as outdated verify that it is both
> offloaded to hardware and doesn't have any pending updates.
> 
> Fixes: 41f2c7c342d3 ("net/sched: act_ct: Fix promotion of offloaded unreplied tuple")
> Reviewed-by: Paul Blakey <paulb@nvidia.com>
> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> I am taking Vlad's patch and rebasing as per his request:
> 
> This patch requires:
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231024193815.1987-1-pablo@netfilter.org/
> 
>   net/sched/act_ct.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 0d44da4e8c8e..fb52d6f9aff9 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -281,6 +281,8 @@ static int tcf_ct_flow_table_fill_actions(struct net *net,
>   static bool tcf_ct_flow_is_outdated(const struct flow_offload *flow)
>   {
>   	return test_bit(IPS_SEEN_REPLY_BIT, &flow->ct->status) &&
> +	       test_bit(IPS_HW_OFFLOAD_BIT, &flow->ct->status) &&
> +	       !test_bit(NF_FLOW_HW_PENDING, &flow->flags) &&
>   	       !test_bit(NF_FLOW_HW_ESTABLISHED, &flow->flags);
>   }
>   

Reviewed-by: Paul Blakey <paulb@nvidia.com>