netdev - Re: [PATCH nf 2/2] netfilter: nf_tables: add clone interface to expression operations

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]

Date:	Tue, 10 Nov 2015 18:30:34 +0000
From:	Patrick McHardy <kaber@...sh.net>
To:	Pablo Neira Ayuso <pablo@...filter.org>
Cc:	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH nf 2/2] netfilter: nf_tables: add clone interface to
 expression operations

On 10.11, Pablo Neira Ayuso wrote:
> With the conversion of the counter expressions to make it percpu, we
> need to clone the percpu memory area, otherwise we crash when using
> counters from flow tables.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> ---
>  include/net/netfilter/nf_tables.h | 16 +++++++++++--
>  net/netfilter/nft_counter.c       | 49 ++++++++++++++++++++++++++++++++-------
>  net/netfilter/nft_dynset.c        |  5 ++--
>  3 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index c9149cc..c186457 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -630,6 +630,8 @@ struct nft_expr_ops {
>  	int				(*validate)(const struct nft_ctx *ctx,
>  						    const struct nft_expr *expr,
>  						    const struct nft_data **data);
> +	int				(*clone)(struct nft_expr *dst,
> +						 const struct nft_expr *src);

The functions and data needed during runtime are deliberately kept together
at the beginning of the structure to avoid having to read the entire thing.
So I'd say this shoud go after ->eval().

> @@ -660,10 +662,20 @@ void nft_expr_destroy(const struct nft_ctx *ctx, struct nft_expr *expr);
>  int nft_expr_dump(struct sk_buff *skb, unsigned int attr,
>  		  const struct nft_expr *expr);
>  
> -static inline void nft_expr_clone(struct nft_expr *dst, struct nft_expr *src)
> +static inline int nft_expr_clone(struct nft_expr *dst, struct nft_expr *src)
>  {
> +	int err;
> +
>  	__module_get(src->ops->type->owner);
> -	memcpy(dst, src, src->ops->size);
> +	if (src->ops->clone) {
> +		memcpy(dst, src, sizeof(*src));

Why copy if we clone? The function should do a full initialization if it is
present I would say.

> +		err = src->ops->clone(dst, src);
> +		if (err < 0)
> +			return err;
> +	} else {
> +		memcpy(dst, src, src->ops->size);
> +	}
> +	return 0;
>  }
>  
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html