[PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage
From: Andrzej Hajda
Date: Wed Feb 17 2016 - 07:42:49 EST
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@xxxxxxxxxxx>
---
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