[PATCH nf 1/2] net: add __netdev_alloc_pcpu_stats() to indicate gfp flags

[PATCH nf 1/2] net: add __netdev_alloc_pcpu_stats() to indicate gfp flags

[Pablo Neira Ayuso]

nf_tables may create percpu counters from the packet path through its
dynamic set instantiation infrastructure, so we need a way to allocate
this through GFP_ATOMIC.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netdevice.h | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2c00772..e9d0c8a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2068,20 +2068,23 @@ struct pcpu_sw_netstats {
 	struct u64_stats_sync   syncp;
 };
 
-#define netdev_alloc_pcpu_stats(type)				\
-({								\
-	typeof(type) __percpu *pcpu_stats = alloc_percpu(type); \
-	if (pcpu_stats)	{					\
-		int __cpu;					\
-		for_each_possible_cpu(__cpu) {			\
-			typeof(type) *stat;			\
-			stat = per_cpu_ptr(pcpu_stats, __cpu);	\
-			u64_stats_init(&stat->syncp);		\
-		}						\
-	}							\
-	pcpu_stats;						\
+#define __netdev_alloc_pcpu_stats(type, gfp)				\
+({									\
+	typeof(type) __percpu *pcpu_stats = alloc_percpu_gfp(type, gfp);\
+	if (pcpu_stats)	{						\
+		int __cpu;						\
+		for_each_possible_cpu(__cpu) {				\
+			typeof(type) *stat;				\
+			stat = per_cpu_ptr(pcpu_stats, __cpu);		\
+			u64_stats_init(&stat->syncp);			\
+		}							\
+	}								\
+	pcpu_stats;							\
 })
 
+#define netdev_alloc_pcpu_stats(type)					\
+	__netdev_alloc_pcpu_stats(type, GFP_KERNEL);
+
 #include <linux/notifier.h>
 
 /* netdevice notifier chain. Please remember to update the rtnetlink
-- 
2.1.4

[PATCH] netfilter: Fix removal of GRE expectation entries created by PPTP

[Pablo Neira Ayuso]

From: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>

The uninitialized tuple structure caused incorrect hash calculation
and the lookup failed.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=106441
Signed-off-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
---
Original patch posted on kernel bugzilla.

 net/ipv4/netfilter/nf_nat_pptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/nf_nat_pptp.c b/net/ipv4/netfilter/nf_nat_pptp.c
index 657d230..b3ca21b 100644
--- a/net/ipv4/netfilter/nf_nat_pptp.c
+++ b/net/ipv4/netfilter/nf_nat_pptp.c
@@ -45,7 +45,7 @@ static void pptp_nat_expected(struct nf_conn *ct,
 	struct net *net = nf_ct_net(ct);
 	const struct nf_conn *master = ct->master;
 	struct nf_conntrack_expect *other_exp;
-	struct nf_conntrack_tuple t;
+	struct nf_conntrack_tuple t = {};
 	const struct nf_ct_pptp_master *ct_pptp_info;
 	const struct nf_nat_pptp *nat_pptp_info;
 	struct nf_nat_range range;
-- 
2.1.4

Re: [PATCH] netfilter: Fix removal of GRE expectation entries created by PPTP

[Pablo Neira Ayuso]

On Tue, Nov 10, 2015 at 05:36:29PM +0100, Pablo Neira Ayuso wrote:
> From: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
> 
> The uninitialized tuple structure caused incorrect hash calculation
> and the lookup failed.

Please, ignore this.

This patch is already in the nf tree, it just slipped through in my
last git send-email.

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

[Pablo Neira Ayuso]

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@netfilter.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);
 	const struct nft_expr_type	*type;
 	void				*data;
 };
@@ -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));
+		err = src->ops->clone(dst, src);
+		if (err < 0)
+			return err;
+	} else {
+		memcpy(dst, src, src->ops->size);
+	}
+	return 0;
 }
 
 /**
diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
index 1067fb4..c7808fc 100644
--- a/net/netfilter/nft_counter.c
+++ b/net/netfilter/nft_counter.c
@@ -47,27 +47,34 @@ static void nft_counter_eval(const struct nft_expr *expr,
 	local_bh_enable();
 }
 
-static int nft_counter_dump(struct sk_buff *skb, const struct nft_expr *expr)
+static void nft_counter_fetch(const struct nft_counter_percpu __percpu *counter,
+			      struct nft_counter *total)
 {
-	struct nft_counter_percpu_priv *priv = nft_expr_priv(expr);
-	struct nft_counter_percpu *cpu_stats;
-	struct nft_counter total;
+	const struct nft_counter_percpu *cpu_stats;
 	u64 bytes, packets;
 	unsigned int seq;
 	int cpu;
 
-	memset(&total, 0, sizeof(total));
+	memset(total, 0, sizeof(*total));
 	for_each_possible_cpu(cpu) {
-		cpu_stats = per_cpu_ptr(priv->counter, cpu);
+		cpu_stats = per_cpu_ptr(counter, cpu);
 		do {
 			seq	= u64_stats_fetch_begin_irq(&cpu_stats->syncp);
 			bytes	= cpu_stats->counter.bytes;
 			packets	= cpu_stats->counter.packets;
 		} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, seq));
 
-		total.packets += packets;
-		total.bytes += bytes;
+		total->packets += packets;
+		total->bytes += bytes;
 	}
+}
+
+static int nft_counter_dump(struct sk_buff *skb, const struct nft_expr *expr)
+{
+	struct nft_counter_percpu_priv *priv = nft_expr_priv(expr);
+	struct nft_counter total;
+
+	nft_counter_fetch(priv->counter, &total);
 
 	if (nla_put_be64(skb, NFTA_COUNTER_BYTES, cpu_to_be64(total.bytes)) ||
 	    nla_put_be64(skb, NFTA_COUNTER_PACKETS, cpu_to_be64(total.packets)))
@@ -118,6 +125,31 @@ static void nft_counter_destroy(const struct nft_ctx *ctx,
 	free_percpu(priv->counter);
 }
 
+static int nft_counter_clone(struct nft_expr *dst, const struct nft_expr *src)
+{
+	struct nft_counter_percpu_priv *priv = nft_expr_priv(src);
+	struct nft_counter_percpu_priv *priv_clone = nft_expr_priv(dst);
+	struct nft_counter_percpu __percpu *cpu_stats;
+	struct nft_counter_percpu *this_cpu;
+	struct nft_counter total;
+
+	nft_counter_fetch(priv->counter, &total);
+
+	cpu_stats = __netdev_alloc_pcpu_stats(struct nft_counter_percpu,
+					      GFP_ATOMIC);
+	if (cpu_stats == NULL)
+		return ENOMEM;
+
+	preempt_disable();
+	this_cpu = this_cpu_ptr(cpu_stats);
+	this_cpu->counter.packets = total.packets;
+	this_cpu->counter.bytes = total.bytes;
+	preempt_enable();
+
+	priv_clone->counter = cpu_stats;
+	return 0;
+}
+
 static struct nft_expr_type nft_counter_type;
 static const struct nft_expr_ops nft_counter_ops = {
 	.type		= &nft_counter_type,
@@ -126,6 +158,7 @@ static const struct nft_expr_ops nft_counter_ops = {
 	.init		= nft_counter_init,
 	.destroy	= nft_counter_destroy,
 	.dump		= nft_counter_dump,
+	.clone		= nft_counter_clone,
 };
 
 static struct nft_expr_type nft_counter_type __read_mostly = {
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 513a8ef..9dec3bd 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -50,8 +50,9 @@ static void *nft_dynset_new(struct nft_set *set, const struct nft_expr *expr,
 	}
 
 	ext = nft_set_elem_ext(set, elem);
-	if (priv->expr != NULL)
-		nft_expr_clone(nft_set_ext_expr(ext), priv->expr);
+	if (priv->expr != NULL &&
+	    nft_expr_clone(nft_set_ext_expr(ext), priv->expr) < 0)
+		return NULL;
 
 	return elem;
 }
-- 
2.1.4

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

[Patrick McHardy]

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@netfilter.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;
>  }
>  

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

[Pablo Neira Ayuso]

On Tue, Nov 10, 2015 at 06:30:34PM +0000, Patrick McHardy wrote:
> 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@netfilter.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().

OK, I'll place this 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.

This is not copying the variable length data area of the expression,
just the expression head.

> > +		err = src->ops->clone(dst, src);
> > +		if (err < 0)
> > +			return err;
> > +	} else {
> > +		memcpy(dst, src, src->ops->size);
> > +	}
> > +	return 0;
> >  }
> >  

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

[Patrick McHardy]

On 10.11, Pablo Neira Ayuso wrote:
> On Tue, Nov 10, 2015 at 06:30:34PM +0000, Patrick McHardy wrote:
> > >  	__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.
> 
> This is not copying the variable length data area of the expression,
> just the expression head.

Ah right. But that is only ->ops. We can set this directly, should generate
better code and be easier to understand.

> 
> > > +		err = src->ops->clone(dst, src);
> > > +		if (err < 0)
> > > +			return err;
> > > +	} else {
> > > +		memcpy(dst, src, src->ops->size);
> > > +	}
> > > +	return 0;
> > >  }
> > >  
> 

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

[Pablo Neira Ayuso]

On Tue, Nov 10, 2015 at 06:58:05PM +0000, Patrick McHardy wrote:
> On 10.11, Pablo Neira Ayuso wrote:
> > On Tue, Nov 10, 2015 at 06:30:34PM +0000, Patrick McHardy wrote:
> > > >  	__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.
> > 
> > This is not copying the variable length data area of the expression,
> > just the expression head.
> 
> Ah right. But that is only ->ops. We can set this directly, should generate
> better code and be easier to understand.

I left the memcpy just to avoid that we forget in case we ever get
more data there (unlikely). So I'll set the pointer instead.

If no further objections, will make those two changes locally and will
push this upstream.

Thanks!

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

[Patrick McHardy]

On 10.11, Pablo Neira Ayuso wrote:
> On Tue, Nov 10, 2015 at 06:58:05PM +0000, Patrick McHardy wrote:
> > On 10.11, Pablo Neira Ayuso wrote:
> > > On Tue, Nov 10, 2015 at 06:30:34PM +0000, Patrick McHardy wrote:
> > > > >  	__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.
> > > 
> > > This is not copying the variable length data area of the expression,
> > > just the expression head.
> > 
> > Ah right. But that is only ->ops. We can set this directly, should generate
> > better code and be easier to understand.
> 
> I left the memcpy just to avoid that we forget in case we ever get
> more data there (unlikely). So I'll set the pointer instead.
> 
> If no further objections, will make those two changes locally and will
> push this upstream.

No further objections :)

Re: [PATCH nf 1/2] net: add __netdev_alloc_pcpu_stats() to indicate gfp flags

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 10 Nov 2015 17:36:28 +0100

> nf_tables may create percpu counters from the packet path through its
> dynamic set instantiation infrastructure, so we need a way to allocate
> this through GFP_ATOMIC.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Acked-by: David S. Miller <davem@davemloft.net>