Re: [PATCH RFC] memcg: Enable accounting for nft objects

[Florian Westphal <fw@strlen.de>]

Vasily Averin <vvs@virtuozzo.com> wrote:
> nftables replaces iptables but still lacks memcg accounting.
> 
> This patch account most part of nft-related allocation and should protect host from nft misuse
> inside memcg-limited container.
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  net/netfilter/core.c          |  2 +-
>  net/netfilter/nf_tables_api.c | 51 +++++++++++++++++++----------------
>  2 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 354cb472f386..6a2b57774999 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -58,7 +58,7 @@ static struct nf_hook_entries *allocate_hook_entries_size(u16 num)
>  	if (num == 0)
>  		return NULL;
> -	e = kvzalloc(alloc, GFP_KERNEL);
> +	e = kvzalloc(alloc, GFP_KERNEL_ACCOUNT);

makes sense to me.

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 5fa16990da95..5e1987ec9715 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -149,7 +149,7 @@ static struct nft_trans *nft_trans_alloc_gfp(const struct nft_ctx *ctx,
>  {
>  	struct nft_trans *trans;
> -	trans = kzalloc(sizeof(struct nft_trans) + size, gfp);
> +	trans = kzalloc(sizeof(struct nft_trans) + size, gfp | __GFP_ACCOUNT);

trans_alloc is temporary in nature, they are always free'd by the
time syscall returns (else, bug).

> @@ -1084,6 +1084,7 @@ static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
>  	struct nft_table *table;
>  	struct nft_ctx ctx;
>  	u32 flags = 0;
> +	gfp_t gfp = GFP_KERNEL_ACCOUNT;
>  	int err;
>  	lockdep_assert_held(&nft_net->commit_mutex);
> @@ -1113,16 +1114,16 @@ static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
>  	}
>  	err = -ENOMEM;
> -	table = kzalloc(sizeof(*table), GFP_KERNEL);
> +	table = kzalloc(sizeof(*table), gfp);

Why gfp temporary variable?  Readability? The subsititution looks correct.

Rest looks good, you might need to update nft_limit_init() and a few
other stateful expressions that alloc internal data too.