netdev - Re: [PATCH net 1/3] netfilter: nf_tables: integrate pipapo into commit protocol

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: <ZILnn2nnIGfUwq1J@corigine.com>
Date: Fri, 9 Jun 2023 10:49:35 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: netfilter-devel@...r.kernel.org, davem@...emloft.net,
	netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com
Subject: Re: [PATCH net 1/3] netfilter: nf_tables: integrate pipapo into
 commit protocol

On Thu, Jun 08, 2023 at 09:57:04PM +0200, Pablo Neira Ayuso wrote:
> The pipapo set backend follows copy-on-update approach, maintaining one
> clone of the existing datastructure that is being updated. The clone
> and current datastructures are swapped via rcu from the commit step.
> 
> The existing integration with the commit protocol is flawed because
> there is no operation to clean up the clone if the transaction is
> aborted. Moreover, the datastructure swap happens on set element
> activation.
> 
> This patch adds two new operations for sets: commit and abort, these new
> operations are invoked from the commit and abort steps, after the
> transactions have been digested, and it updates the pipapo set backend
> to use it.
> 
> This patch adds a new ->pending_update field to sets to maintain a list
> of sets that require this new commit and abort operations.
> 
> Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>

Hi Pablo,

some suggestions of some trivial follow-up items from my side.
No need for these to hold up progress of the patchset.

> ---
>  include/net/netfilter/nf_tables.h |  4 ++-
>  net/netfilter/nf_tables_api.c     | 56 +++++++++++++++++++++++++++++++
>  net/netfilter/nft_set_pipapo.c    | 55 +++++++++++++++++++++---------
>  3 files changed, 99 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 2e24ea1d744c..83db182decc8 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -462,7 +462,8 @@ struct nft_set_ops {
>  					       const struct nft_set *set,
>  					       const struct nft_set_elem *elem,
>  					       unsigned int flags);
> -
> +	void				(*commit)(const struct nft_set *set);
> +	void				(*abort)(const struct nft_set *set);

As a follow-up, these could be added to the kdoc for nft_set_ops.

>  	u64				(*privsize)(const struct nlattr * const nla[],
>  						    const struct nft_set_desc *desc);
>  	bool				(*estimate)(const struct nft_set_desc *desc,
> @@ -557,6 +558,7 @@ struct nft_set {
>  	u16				policy;
>  	u16				udlen;
>  	unsigned char			*udata;
> +	struct list_head		pending_update;

Likewise, as a follow-up, pending_update could be added to the kdoc for
nft_set.

>  	/* runtime data below here */
>  	const struct nft_set_ops	*ops ____cacheline_aligned;
>  	u16				flags:14,

...