Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334487 > unrolled thread

[PATCH 1/7] netfilter: fix IS_ERR_VALUE usage

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-15 15:40 +0100
    Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-17 03:40 +0100
      Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 09:50 +0100
      [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 13:50 +0100
        Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Arnd Bergmann <arnd@arndb.de> - 2016-02-17 14:50 +0100
          Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 16:00 +0100
            Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Arnd Bergmann <arnd@arndb.de> - 2016-02-17 16:50 +0100

#1334487 — [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage

IS_ERR_VALUE should be used only with unsigned long type.
Otherwise it can work incorrectly. To achieve this function
xt_percpu_counter_alloc is modified to return unsigned long,
and its result is assigned to temporary variable to perform
error checking, before assigning to .pcnt field.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 include/linux/netfilter/x_tables.h |  6 +++---
 net/ipv4/netfilter/arp_tables.c    | 11 +++++++----
 net/ipv4/netfilter/ip_tables.c     | 12 ++++++++----
 net/ipv6/netfilter/ip6_tables.c    | 13 +++++++++----
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index c557741..79d4306 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -370,16 +370,16 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
  * allows us to return 0 for single core systems without forcing
  * callers to deal with SMP vs. NONSMP issues.
  */
-static inline u64 xt_percpu_counter_alloc(void)
+static inline unsigned long xt_percpu_counter_alloc(void)
 {
 	if (nr_cpu_ids > 1) {
 		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
 						    sizeof(struct xt_counters));
 
 		if (res == NULL)
-			return (u64) -ENOMEM;
+			return -ENOMEM;
 
-		return (u64) (__force unsigned long) res;
+		return (__force unsigned long) res;
 	}
 
 	return 0;
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac..6dcc208 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -521,14 +521,16 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 	struct xt_entry_target *t;
 	struct xt_target *target;
 	int ret;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	t = arpt_get_target(e);
 	target = xt_request_find_target(NFPROTO_ARP, t->u.user.name,
@@ -1423,11 +1425,12 @@ static int translate_compat_table(const char *name,
 
 	i = 0;
 	xt_entry_foreach(iter1, entry1, newinfo->size) {
-		iter1->counters.pcnt = xt_percpu_counter_alloc();
-		if (IS_ERR_VALUE(iter1->counters.pcnt)) {
+		unsigned long pcnt = xt_percpu_counter_alloc();
+		if (IS_ERR_VALUE(pcnt)) {
 			ret = -ENOMEM;
 			break;
 		}
+		iter1->counters.pcnt = pcnt;
 
 		ret = check_target(iter1, name);
 		if (ret != 0) {
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..ad57c78 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -665,14 +665,16 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1609,10 +1611,12 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 	struct xt_mtchk_param mtpar;
 	unsigned int j;
 	int ret = 0;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf..4291c8d 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -678,14 +678,16 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1619,10 +1621,13 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 	int ret = 0;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
+
 	j = 0;
 	mtpar.net	= net;
 	mtpar.table     = name;
-- 
1.9.1

[toc] | [next] | [standalone]

#1336017

On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote:
> IS_ERR_VALUE should be used only with unsigned long type.
> Otherwise it can work incorrectly. To achieve this function
> xt_percpu_counter_alloc is modified to return unsigned long,
> and its result is assigned to temporary variable to perform
> error checking, before assigning to .pcnt field.

	Wrong fix, IMO.  Just have an anon union of u64 pcnt and
struct xt_counters __percpu *pcpu in there.  And make this

> +static inline unsigned long xt_percpu_counter_alloc(void)
>  {
>  	if (nr_cpu_ids > 1) {
>  		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
>  						    sizeof(struct xt_counters));
>  
>  		if (res == NULL)
> -			return (u64) -ENOMEM;
> +			return -ENOMEM;
>  
> -		return (u64) (__force unsigned long) res;
> +		return (__force unsigned long) res;
>  	}
>  
>  	return 0;

take struct xt_counters * and return 0 or -ENOMEM.  Storing the result of
allocation in ->pcpu of passed structure.

I mean, look at the callers -

> -	e->counters.pcnt = xt_percpu_counter_alloc();
> -	if (IS_ERR_VALUE(e->counters.pcnt))
> +	pcnt = xt_percpu_counter_alloc();
> +	if (IS_ERR_VALUE(pcnt))
>  		return -ENOMEM;
> +	e->counters.pcnt = pcnt;

should be
	if (xt_percpu_counter_alloc(&e->counters) < 0)
		return -ENOMEM;

and similar for the rest of callers.  Moreover, if you look at the _users_
of that fields, you'll see that a bunch of those actually want to use
->pcpu instead of doing all those casts.

Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need
to figure out what's going on in that place", which does include reading
through the code.  Mechanical "solutions" like that only hide the problem.

[toc] | [prev] | [next] | [standalone]

#1336143

On 02/17/2016 03:31 AM, Al Viro wrote:
> On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote:
>> IS_ERR_VALUE should be used only with unsigned long type.
>> Otherwise it can work incorrectly. To achieve this function
>> xt_percpu_counter_alloc is modified to return unsigned long,
>> and its result is assigned to temporary variable to perform
>> error checking, before assigning to .pcnt field.
> 	Wrong fix, IMO.  Just have an anon union of u64 pcnt and
> struct xt_counters __percpu *pcpu in there.  And make this
>
>> +static inline unsigned long xt_percpu_counter_alloc(void)
>>  {
>>  	if (nr_cpu_ids > 1) {
>>  		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
>>  						    sizeof(struct xt_counters));
>>  
>>  		if (res == NULL)
>> -			return (u64) -ENOMEM;
>> +			return -ENOMEM;
>>  
>> -		return (u64) (__force unsigned long) res;
>> +		return (__force unsigned long) res;
>>  	}
>>  
>>  	return 0;
> take struct xt_counters * and return 0 or -ENOMEM.  Storing the result of
> allocation in ->pcpu of passed structure.
>
> I mean, look at the callers -
>
>> -	e->counters.pcnt = xt_percpu_counter_alloc();
>> -	if (IS_ERR_VALUE(e->counters.pcnt))
>> +	pcnt = xt_percpu_counter_alloc();
>> +	if (IS_ERR_VALUE(pcnt))
>>  		return -ENOMEM;
>> +	e->counters.pcnt = pcnt;
> should be
> 	if (xt_percpu_counter_alloc(&e->counters) < 0)
> 		return -ENOMEM;
>
> and similar for the rest of callers.  Moreover, if you look at the _users_
> of that fields, you'll see that a bunch of those actually want to use
> ->pcpu instead of doing all those casts.
>
> Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need
> to figure out what's going on in that place", which does include reading
> through the code.  Mechanical "solutions" like that only hide the problem.
>
>
I just tried to make the patch the least invasive :)

The problem with your proposition is that struct xt_counters is exposed
to userspace as well as the structs containing it:
struct arpt_entry,
struct ipt_entry,
struct ip6t_entry

Mixing __percpu pointer into these structures seems problematic.
Maybe it would be better to skip adding union and do ugly casting
in xt_percpu_counter_alloc(struct xt_counters *) and friends.

Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1336398 — [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

IS_ERR_VALUE should be used only with unsigned long type. Otherwise
it can work incorrectly. To achieve this function xt_percpu_counter_alloc
is modified to return only error code, pointer to counters is passed as an
argument. Helper union have been created to avoid ugly typecasting and
make code more readable.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi Al,

This is prettier version, at least in my opinion :)
It uses external union to avoid touching uapi structures.

Regards
Andrzej

 include/linux/netfilter/x_tables.h | 41 ++++++++++++++++++++------------------
 net/ipv4/netfilter/arp_tables.c    | 18 ++++++++---------
 net/ipv4/netfilter/ip_tables.c     | 20 +++++++++----------
 net/ipv6/netfilter/ip6_tables.c    | 21 +++++++++----------
 4 files changed, 51 insertions(+), 49 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index c557741..82faecb 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -357,44 +357,47 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
 	return ret;
 }
 
-
-/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
- * real (percpu) counter.  On !SMP, its just the packet count,
- * so nothing needs to be done there.
- *
- * xt_percpu_counter_alloc returns the address of the percpu
- * counter, or 0 on !SMP. We force an alignment of 16 bytes
- * so that bytes/packets share a common cache line.
- *
- * Hence caller must use IS_ERR_VALUE to check for error, this
- * allows us to return 0 for single core systems without forcing
- * callers to deal with SMP vs. NONSMP issues.
+/*
+ * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
+ * counter.  On !SMP, it is just the packet count. union ext_counters is used
+ * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
+ * structures as these are exposed to userspace.
  */
-static inline u64 xt_percpu_counter_alloc(void)
+union xt_smp_counters {
+	struct xt_counters counters;
+	struct xt_counters __percpu *smp_counters;
+};
+
+static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
+{
+	return container_of(cnt, union xt_smp_counters, counters);
+}
+
+static inline int xt_percpu_counter_alloc(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1) {
 		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
 						    sizeof(struct xt_counters));
 
 		if (res == NULL)
-			return (u64) -ENOMEM;
+			return -ENOMEM;
 
-		return (u64) (__force unsigned long) res;
+		to_xt_smp_counters(cnt)->smp_counters = res;
 	}
 
 	return 0;
 }
-static inline void xt_percpu_counter_free(u64 pcnt)
+static inline void xt_percpu_counter_free(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1)
-		free_percpu((void __percpu *) (unsigned long) pcnt);
+		free_percpu(to_xt_smp_counters(cnt)->smp_counters);
 }
 
 static inline struct xt_counters *
 xt_get_this_cpu_counter(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1)
-		return this_cpu_ptr((void __percpu *) (unsigned long) cnt->pcnt);
+		return this_cpu_ptr(to_xt_smp_counters(cnt)->smp_counters);
 
 	return cnt;
 }
@@ -403,7 +406,7 @@ static inline struct xt_counters *
 xt_get_per_cpu_counter(struct xt_counters *cnt, unsigned int cpu)
 {
 	if (nr_cpu_ids > 1)
-		return per_cpu_ptr((void __percpu *) (unsigned long) cnt->pcnt, cpu);
+		return per_cpu_ptr(to_xt_smp_counters(cnt)->smp_counters, cpu);
 
 	return cnt;
 }
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac..be589e5 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -526,9 +526,9 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	t = arpt_get_target(e);
 	target = xt_request_find_target(NFPROTO_ARP, t->u.user.name,
@@ -547,7 +547,7 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 err:
 	module_put(t->u.kernel.target->me);
 out:
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -625,7 +625,7 @@ static inline void cleanup_entry(struct arpt_entry *e)
 	if (par.target->destroy != NULL)
 		par.target->destroy(&par);
 	module_put(par.target->me);
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1423,15 +1423,13 @@ static int translate_compat_table(const char *name,
 
 	i = 0;
 	xt_entry_foreach(iter1, entry1, newinfo->size) {
-		iter1->counters.pcnt = xt_percpu_counter_alloc();
-		if (IS_ERR_VALUE(iter1->counters.pcnt)) {
-			ret = -ENOMEM;
+		ret = xt_percpu_counter_alloc(&iter1->counters);
+		if (ret < 0)
 			break;
-		}
 
 		ret = check_target(iter1, name);
 		if (ret != 0) {
-			xt_percpu_counter_free(iter1->counters.pcnt);
+			xt_percpu_counter_free(iter1->counters);
 			break;
 		}
 		++i;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..5f3f96b 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -670,9 +670,9 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -711,7 +711,7 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -797,7 +797,7 @@ cleanup_entry(struct ipt_entry *e, struct net *net)
 	if (par.target->destroy != NULL)
 		par.target->destroy(&par);
 	module_put(par.target->me);
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1608,11 +1608,11 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 	struct xt_entry_match *ematch;
 	struct xt_mtchk_param mtpar;
 	unsigned int j;
-	int ret = 0;
+	int ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1639,7 +1639,7 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf..25b6a90 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -683,9 +683,9 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -723,7 +723,7 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -809,7 +809,7 @@ static void cleanup_entry(struct ip6t_entry *e, struct net *net)
 		par.target->destroy(&par);
 	module_put(par.target->me);
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1616,13 +1616,14 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 			      const char *name)
 {
 	unsigned int j;
-	int ret = 0;
+	int ret;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
+
 	j = 0;
 	mtpar.net	= net;
 	mtpar.table     = name;
@@ -1648,7 +1649,7 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1336440 — Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

On Wednesday 17 February 2016 13:41:29 Andrzej Hajda wrote:
> IS_ERR_VALUE should be used only with unsigned long type. Otherwise
> it can work incorrectly. To achieve this function xt_percpu_counter_alloc
> is modified to return only error code, pointer to counters is passed as an
> argument. Helper union have been created to avoid ugly typecasting and
> make code more readable.
> 
> The patch follows conclusion from discussion on LKML [1][2].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

I think it would be helpful to mention here how the current code is
actually broken, i.e. that we set the u64 value to (u64)-ENOMEM
on failure but then compare it to (unsigned long)-MAX_ERRNO, which
is much smaller on a 32-bit architecture, and basically relies on
never even needing the range of the u64 variable.

It works because we only do this comparison at allocation time, while
in the non-SMP case it might be larger than (unsigned long)-MAX_ERRNO
later but then we don't do the IS_ERR_VALUE comparison any more.

> -/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
> - * real (percpu) counter.  On !SMP, its just the packet count,
> - * so nothing needs to be done there.
> - *
> - * xt_percpu_counter_alloc returns the address of the percpu
> - * counter, or 0 on !SMP. We force an alignment of 16 bytes
> - * so that bytes/packets share a common cache line.
> - *
> - * Hence caller must use IS_ERR_VALUE to check for error, this
> - * allows us to return 0 for single core systems without forcing
> - * callers to deal with SMP vs. NONSMP issues.
> +/*
> + * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
> + * counter.  On !SMP, it is just the packet count. union ext_counters is used
> + * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
> + * structures as these are exposed to userspace.
>   */
> -static inline u64 xt_percpu_counter_alloc(void)
> +union xt_smp_counters {
> +	struct xt_counters counters;
> +	struct xt_counters __percpu *smp_counters;
> +};
> +
> +static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
> +{
> +	return container_of(cnt, union xt_smp_counters, counters);
> +}

The union is a bit ugly, but I can't think of a much better
way to do this.

However, could you put the union into the three users (struct arpt_entry
etc) to avoid having to cast the inner structure into the union using
container_of()? It doesn't feel right to use container_of() in this
way here.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1336501 — Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

On 02/17/2016 02:42 PM, Arnd Bergmann wrote:
> On Wednesday 17 February 2016 13:41:29 Andrzej Hajda wrote:
>> IS_ERR_VALUE should be used only with unsigned long type. Otherwise
>> it can work incorrectly. To achieve this function xt_percpu_counter_alloc
>> is modified to return only error code, pointer to counters is passed as an
>> argument. Helper union have been created to avoid ugly typecasting and
>> make code more readable.
>>
>> The patch follows conclusion from discussion on LKML [1][2].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
>> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581
> I think it would be helpful to mention here how the current code is
> actually broken, i.e. that we set the u64 value to (u64)-ENOMEM
> on failure but then compare it to (unsigned long)-MAX_ERRNO, which
> is much smaller on a 32-bit architecture, and basically relies on
> never even needing the range of the u64 variable.
>
> It works because we only do this comparison at allocation time, while
> in the non-SMP case it might be larger than (unsigned long)-MAX_ERRNO
> later but then we don't do the IS_ERR_VALUE comparison any more.
>
>> -/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
>> - * real (percpu) counter.  On !SMP, its just the packet count,
>> - * so nothing needs to be done there.
>> - *
>> - * xt_percpu_counter_alloc returns the address of the percpu
>> - * counter, or 0 on !SMP. We force an alignment of 16 bytes
>> - * so that bytes/packets share a common cache line.
>> - *
>> - * Hence caller must use IS_ERR_VALUE to check for error, this
>> - * allows us to return 0 for single core systems without forcing
>> - * callers to deal with SMP vs. NONSMP issues.
>> +/*
>> + * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
>> + * counter.  On !SMP, it is just the packet count. union ext_counters is used
>> + * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
>> + * structures as these are exposed to userspace.
>>   */
>> -static inline u64 xt_percpu_counter_alloc(void)
>> +union xt_smp_counters {
>> +	struct xt_counters counters;
>> +	struct xt_counters __percpu *smp_counters;
>> +};
>> +
>> +static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
>> +{
>> +	return container_of(cnt, union xt_smp_counters, counters);
>> +}
> The union is a bit ugly, but I can't think of a much better
> way to do this.
>
> However, could you put the union into the three users (struct arpt_entry
> etc) to avoid having to cast the inner structure into the union using
> container_of()? It doesn't feel right to use container_of() in this
> way here.
>
> 	Arnd
>
>
I am not sure if you are aware of the fact these structures are exposed
to user
space. Is it OK to add such unions to them?


Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1336530 — Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

On Wednesday 17 February 2016 15:54:11 Andrzej Hajda wrote:
> > However, could you put the union into the three users (struct arpt_entry
> > etc) to avoid having to cast the inner structure into the union using
> > container_of()? It doesn't feel right to use container_of() in this
> > way here.
> >
> >
> I am not sure if you are aware of the fact these structures are exposed
> to user
> space. Is it OK to add such unions to them?
> 

You are right, that would be odd. My first idea was actually to put
a union into struct xt_counters, and I did notice that this was
exposed to user space so I did not mention it.

Putting a union into arpt_entry etc would be worse then. The only
alternative I see would be to define xt_counters as

struct xt_counters {
#ifndef __KERNEL__
        __u64 pcnt, bcnt;                       /* Packet and byte counters */
#else
	union {
		__u64 pcnt;
		struct xt_counters __percpu *xt_smp_counters;
	};
	__u64 bcnt;
#endif
};

but that is still really ugly, and no real improvement over your
approach.

One really simple fix would be to basically open-code a correct
version of IS_ERR_VALUE specifically for xt_counters and leave
everything using the __u64 hack:

-static inline u64 xt_percpu_counter_alloc(void)
+static inline int xt_percpu_counter_alloc(struct xt_counters *cnt)
 {
        if (nr_cpu_ids > 1) {
                void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
                                                    sizeof(struct xt_counters));
 
                if (res == NULL)
-                       return (u64) -ENOMEM;
+                       return -ENOMEM;
 
-               return (u64) (__force unsigned long) res;
+               cnt->pcnt = (u64)(uintptr_t)res;
        }
 
        return 0;
 }

that avoids the union but keeps the implicit overloading of the
pcnt field, just local to the alloc/free functions.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web