From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965305AbcBQMmp (ORCPT ); Wed, 17 Feb 2016 07:42:45 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:8874 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965099AbcBQMmn (ORCPT ); Wed, 17 Feb 2016 07:42:43 -0500 X-AuditID: cbfec7f4-f79026d00000418a-e5-56c46ac28b15 From: Andrzej Hajda To: linux-kernel@vger.kernel.org Cc: Andrzej Hajda , Bartlomiej Zolnierkiewicz , Marek Szyprowski , arnd@arndb.de, viro@ZenIV.linux.org.uk Subject: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Date: Wed, 17 Feb 2016 13:41:29 +0100 Message-id: <1455712889-10633-1-git-send-email-a.hajda@samsung.com> X-Mailer: git-send-email 1.9.1 In-reply-to: <20160217023127.GG17997@ZenIV.linux.org.uk> References: <20160217023127.GG17997@ZenIV.linux.org.uk> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupiluLIzCtJLcpLzFFi42I5/e/4Nd1DWUfCDDo+M1ncWneO1eLvpGPs FhtnrGe1uLxrDpvF2iN32S3O/z3O6sDm8fvXJEaPvi2rGD0+b5Lz2PTkLVMASxSXTUpqTmZZ apG+XQJXxstff1kL+twrtve0MTUw7rbqYuTkkBAwkfh76DYzhC0mceHeerYuRi4OIYGljBIr Jk1mhnCamCQuvLjEAlLFJqAp8XfzTTYQW0RAQWJz7zNWkCJmgR2MEu86z4IlhAUsJc7+f8MK YrMIqEqs/7cAzOYVcJa4tOYuK8Q6OYmTxyaD2ZwCFhLvXi9n72LkANpmLvHlddAERt4FjAyr GEVTS5MLipPScw31ihNzi0vz0vWS83M3MUJC6MsOxsXHrA4xCnAwKvHwBmQcDhNiTSwrrsw9 xCjBwawkwluYdiRMiDclsbIqtSg/vqg0J7X4EKM0B4uSOO/cXe9DhATSE0tSs1NTC1KLYLJM HJxSDYyqPznOJWg9EOr5eCJOfbrd3/Z/Dd0Vc+bErdNKqZwQsUCOa+mPWR0ddyyyvzVmzWc6 7GAf/Y+1fHrqmgbj3RNe71fjtkxcoZNZd962hb1BdbZvXmNj0s8jwlkLfqt876jqjV+WzjZr 85J11S3mvpeKOtfMszdi/ZYR8m/PifCVe+8c8tt2b4kSS3FGoqEWc1FxIgDT3rFdHQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- 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