netdev - Re: [PATCH net] netfilter: flowtable: Add pending bit for offload work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200510221434.GA11226@salvia>
Date:   Mon, 11 May 2020 00:14:34 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Paul Blakey <paulb@...lanox.com>
Cc:     Oz Shlomo <ozsh@...lanox.com>, Roi Dayan <roid@...lanox.com>,
        netdev@...r.kernel.org, Saeed Mahameed <saeedm@...lanox.com>,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net] netfilter: flowtable: Add pending bit for offload
 work

Hi,

On Wed, May 06, 2020 at 02:24:39PM +0300, Paul Blakey wrote:
> Gc step can queue offloaded flow del work or stats work.
> Those work items can race each other and a flow could be freed
> before the stats work is executed and querying it.
> To avoid that, add a pending bit that if a work exists for a flow
> don't queue another work for it.
> This will also avoid adding multiple stats works in case stats work
> didn't complete but gc step started again.

This is happening since the mutex has been removed, right?

Another question below.

> Signed-off-by: Paul Blakey <paulb@...lanox.com>
> Reviewed-by: Roi Dayan <roid@...lanox.com>
> ---
>  include/net/netfilter/nf_flow_table.h | 1 +
>  net/netfilter/nf_flow_table_offload.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index 6bf6965..c54a7f7 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -127,6 +127,7 @@ enum nf_flow_flags {
>  	NF_FLOW_HW_DYING,
>  	NF_FLOW_HW_DEAD,
>  	NF_FLOW_HW_REFRESH,
> +	NF_FLOW_HW_PENDING,
>  };
>  
>  enum flow_offload_type {
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index b9d5ecc..731d738 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -817,6 +817,7 @@ static void flow_offload_work_handler(struct work_struct *work)
>  			WARN_ON_ONCE(1);
>  	}
>  
> +	clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
>  	kfree(offload);
>  }
>  
> @@ -831,9 +832,14 @@ static void flow_offload_queue_work(struct flow_offload_work *offload)
>  {
>  	struct flow_offload_work *offload;
>  
> +	if (test_and_set_bit(NF_FLOW_HW_PENDING, &flow->flags))
> +		return NULL;

In case of stats, it's fine to lose work.

But how does this work for the deletion case? Does this falls back to
the timeout deletion?

Thanks.