| Message ID | 1455546925-22119-2-git-send-email-a.hajda@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)