* [PATCH 0/7] fix IS_ERR_VALUE usage @ 2016-02-15 14:35 Andrzej Hajda 2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda ` (7 more replies) 0 siblings, 8 replies; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, coreteam, linux-arm-kernel, linux-fbdev, linux-media, linux-mips, linuxppc-dev, linux-samsung-soc, linux-serial, linux-usb, netdev, netfilter-devel, Bartlomiej Zolnierkiewicz, Marek Szyprowski Hi, This small set of independent patches tries to fix incorrect IS_ERR_VALUE macro usage. It fixes most usages leading to errors as described in [1]. It also follows conclusion from the discussion [1][2] - IS_ERR_VALUE should be used only with unsigned long type, signed types should use comparison 'ret < 0'. The patchset does not fix errors present in net/ethernet/freescale and soc/fsq/qe drivers - these drivers mixes different types: dma_addr_t, u32, unsigned long, fixing it properly seems to me more challenging, maybe maintainers or brave volunteers can look it. The list of missing fixes: drivers/net/ethernet/freescale/fs_enet/mac-scc.c:149:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(fep -> ring_mem_addr) drivers/net/ethernet/freescale/ucc_geth.c:2237:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_bd_ring_offset [ j ]) drivers/net/ethernet/freescale/ucc_geth.c:2314:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_ring_offset [ j ]) drivers/net/ethernet/freescale/ucc_geth.c:2524:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_glbl_pram_offset) drivers/net/ethernet/freescale/ucc_geth.c:2544:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_tx_offset) drivers/net/ethernet/freescale/ucc_geth.c:2571:46-47: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> send_q_mem_reg_offset) drivers/net/ethernet/freescale/ucc_geth.c:2612:42-43: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> scheduler_offset) drivers/net/ethernet/freescale/ucc_geth.c:2659:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_fw_statistics_pram_offset) drivers/net/ethernet/freescale/ucc_geth.c:2696:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_glbl_pram_offset) drivers/net/ethernet/freescale/ucc_geth.c:2715:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_rx_offset) drivers/net/ethernet/freescale/ucc_geth.c:2736:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_fw_statistics_pram_offset) drivers/net/ethernet/freescale/ucc_geth.c:2756:53-54: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_irq_coalescing_tbl_offset) drivers/net/ethernet/freescale/ucc_geth.c:2822:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_qs_tbl_offset) drivers/net/ethernet/freescale/ucc_geth.c:2908:47-48: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> exf_glbl_param_offset) drivers/net/ethernet/freescale/ucc_geth.c:292:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_offset) drivers/net/ethernet/freescale/ucc_geth.c:3042:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_pram_offset) drivers/soc/fsl/qe/ucc_fast.c:271:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_tx_virtual_fifo_base_offset) drivers/soc/fsl/qe/ucc_fast.c:284:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_rx_virtual_fifo_base_offset) drivers/soc/fsl/qe/ucc_slow.c:186:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> us_pram_offset) drivers/soc/fsl/qe/ucc_slow.c:213:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> rx_base_offset) drivers/soc/fsl/qe/ucc_slow.c:224:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> tx_base_offset) drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset) [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927 [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581 Regards Andrzej Andrzej Hajda (7): netfilter: fix IS_ERR_VALUE usage MIPS: module: fix incorrect IS_ERR_VALUE macro usages drivers: char: mem: fix IS_ERROR_VALUE usage atmel-isi: fix IS_ERR_VALUE usage serial: clps711x: fix IS_ERR_VALUE usage fbdev: exynos: fix IS_ERR_VALUE usage usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage arch/mips/kernel/module-rela.c | 2 +- arch/mips/kernel/module.c | 2 +- drivers/char/mem.c | 2 +- drivers/media/platform/soc_camera/atmel-isi.c | 4 ++-- drivers/tty/serial/clps711x.c | 14 ++++++++------ drivers/usb/gadget/udc/fsl_qe_udc.c | 2 +- drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 6 +++--- 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 +++++++++---- 11 files changed, 44 insertions(+), 30 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-17 2:31 ` Al Viro 2016-02-15 14:35 ` [PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages Andrzej Hajda ` (6 subsequent siblings) 7 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, netfilter-devel, coreteam 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda @ 2016-02-17 2:31 ` Al Viro 2016-02-17 8:45 ` Andrzej Hajda 2016-02-17 12:41 ` [PATCH v2 " Andrzej Hajda 0 siblings, 2 replies; 20+ messages in thread From: Al Viro @ 2016-02-17 2:31 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, netfilter-devel, coreteam 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-17 2:31 ` Al Viro @ 2016-02-17 8:45 ` Andrzej Hajda 2016-02-17 12:41 ` [PATCH v2 " Andrzej Hajda 1 sibling, 0 replies; 20+ messages in thread From: Andrzej Hajda @ 2016-02-17 8:45 UTC (permalink / raw) To: Al Viro Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, netfilter-devel, coreteam 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-17 2:31 ` Al Viro 2016-02-17 8:45 ` Andrzej Hajda @ 2016-02-17 12:41 ` Andrzej Hajda 2016-02-17 13:42 ` Arnd Bergmann 1 sibling, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-17 12:41 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, arnd, viro 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-17 12:41 ` [PATCH v2 " Andrzej Hajda @ 2016-02-17 13:42 ` Arnd Bergmann 2016-02-17 14:54 ` Andrzej Hajda 0 siblings, 1 reply; 20+ messages in thread From: Arnd Bergmann @ 2016-02-17 13:42 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, viro 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-17 13:42 ` Arnd Bergmann @ 2016-02-17 14:54 ` Andrzej Hajda 2016-02-17 15:40 ` Arnd Bergmann 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-17 14:54 UTC (permalink / raw) To: Arnd Bergmann Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, viro 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage 2016-02-17 14:54 ` Andrzej Hajda @ 2016-02-17 15:40 ` Arnd Bergmann 0 siblings, 0 replies; 20+ messages in thread From: Arnd Bergmann @ 2016-02-17 15:40 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, viro 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda 2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-15 14:35 ` [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage Andrzej Hajda ` (5 subsequent siblings) 7 siblings, 0 replies; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Ralf Baechle, linux-mips IS_ERR_VALUE macro should be used only with unsigned long type. Specifically it works incorrectly with longer types. 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> --- arch/mips/kernel/module-rela.c | 2 +- arch/mips/kernel/module.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/mips/kernel/module-rela.c b/arch/mips/kernel/module-rela.c index 2b70723..08100dc 100644 --- a/arch/mips/kernel/module-rela.c +++ b/arch/mips/kernel/module-rela.c @@ -125,7 +125,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab, /* This is the symbol it is referring to */ sym = (Elf_Sym *)sechdrs[symindex].sh_addr + ELF_MIPS_R_SYM(rel[i]); - if (IS_ERR_VALUE(sym->st_value)) { + if (sym->st_value >= -MAX_ERRNO) { /* Ignore unresolved weak symbol */ if (ELF_ST_BIND(sym->st_info) == STB_WEAK) continue; diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c index 1833f51..2ba73bf4 100644 --- a/arch/mips/kernel/module.c +++ b/arch/mips/kernel/module.c @@ -214,7 +214,7 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab, /* This is the symbol it is referring to */ sym = (Elf_Sym *)sechdrs[symindex].sh_addr + ELF_MIPS_R_SYM(rel[i]); - if (IS_ERR_VALUE(sym->st_value)) { + if (sym->st_value >= -MAX_ERRNO) { /* Ignore unresolved weak symbol */ if (ELF_ST_BIND(sym->st_info) == STB_WEAK) continue; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda 2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda 2016-02-15 14:35 ` [PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda ` (4 subsequent siblings) 7 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Arnd Bergmann, Greg Kroah-Hartman IS_ERR_VALUE macro should be used only with unsigned long type. Specifically it works incorrectly with longer types. 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> --- drivers/char/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 4f6f94c..71025c2 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -695,7 +695,7 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig) offset += file->f_pos; case SEEK_SET: /* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */ - if (IS_ERR_VALUE((unsigned long long)offset)) { + if ((unsigned long long)offset >= -MAX_ERRNO) { ret = -EOVERFLOW; break; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage 2016-02-15 14:35 ` [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage Andrzej Hajda @ 2016-02-17 2:33 ` Al Viro 0 siblings, 0 replies; 20+ messages in thread From: Al Viro @ 2016-02-17 2:33 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Arnd Bergmann, Greg Kroah-Hartman On Mon, Feb 15, 2016 at 03:35:21PM +0100, Andrzej Hajda wrote: > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > index 4f6f94c..71025c2 100644 > --- a/drivers/char/mem.c > +++ b/drivers/char/mem.c > @@ -695,7 +695,7 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig) > offset += file->f_pos; > case SEEK_SET: > /* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */ > - if (IS_ERR_VALUE((unsigned long long)offset)) { > + if ((unsigned long long)offset >= -MAX_ERRNO) { > ret = -EOVERFLOW; > break; > } ACK ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda ` (2 preceding siblings ...) 2016-02-15 14:35 ` [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-21 16:04 ` Guennadi Liakhovetski 2016-02-15 14:35 ` [PATCH 5/7] serial: clps711x: " Andrzej Hajda ` (3 subsequent siblings) 7 siblings, 2 replies; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Ludovic Desroches, Guennadi Liakhovetski, Mauro Carvalho Chehab, linux-media IS_ERR_VALUE macro should be used only with unsigned long type. For signed types comparison 'ret < 0' should be used. 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> --- drivers/media/platform/soc_camera/atmel-isi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c index 1af779e..ab2d9b9 100644 --- a/drivers/media/platform/soc_camera/atmel-isi.c +++ b/drivers/media/platform/soc_camera/atmel-isi.c @@ -1026,7 +1026,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi, static int atmel_isi_probe(struct platform_device *pdev) { - unsigned int irq; + int irq; struct atmel_isi *isi; struct resource *regs; int ret, i; @@ -1086,7 +1086,7 @@ static int atmel_isi_probe(struct platform_device *pdev) isi->width_flags |= 1 << 9; irq = platform_get_irq(pdev, 0); - if (IS_ERR_VALUE(irq)) { + if (irq < 0) { ret = irq; goto err_req_irq; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage 2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda @ 2016-02-17 2:33 ` Al Viro 2016-02-21 16:04 ` Guennadi Liakhovetski 1 sibling, 0 replies; 20+ messages in thread From: Al Viro @ 2016-02-17 2:33 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Ludovic Desroches, Guennadi Liakhovetski, Mauro Carvalho Chehab, linux-media On Mon, Feb 15, 2016 at 03:35:22PM +0100, Andrzej Hajda wrote: > IS_ERR_VALUE macro should be used only with unsigned long type. > For signed types comparison 'ret < 0' should be used. > > The patch follows conclusion from discussion on LKML [1][2]. ACK ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage 2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda 2016-02-17 2:33 ` Al Viro @ 2016-02-21 16:04 ` Guennadi Liakhovetski 1 sibling, 0 replies; 20+ messages in thread From: Guennadi Liakhovetski @ 2016-02-21 16:04 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Ludovic Desroches, Mauro Carvalho Chehab, linux-media Hi Andrzej, On Mon, 15 Feb 2016, Andrzej Hajda wrote: > IS_ERR_VALUE macro should be used only with unsigned long type. > For signed types comparison 'ret < 0' should be used. > > 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> Thanks for the patch, but this one https://lkml.org/lkml/2016/2/9/392 came a couple of days earlier. Unless there is an important reason to use yours, I'll use that one. Thanks Guennadi > --- > drivers/media/platform/soc_camera/atmel-isi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c > index 1af779e..ab2d9b9 100644 > --- a/drivers/media/platform/soc_camera/atmel-isi.c > +++ b/drivers/media/platform/soc_camera/atmel-isi.c > @@ -1026,7 +1026,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi, > > static int atmel_isi_probe(struct platform_device *pdev) > { > - unsigned int irq; > + int irq; > struct atmel_isi *isi; > struct resource *regs; > int ret, i; > @@ -1086,7 +1086,7 @@ static int atmel_isi_probe(struct platform_device *pdev) > isi->width_flags |= 1 << 9; > > irq = platform_get_irq(pdev, 0); > - if (IS_ERR_VALUE(irq)) { > + if (irq < 0) { > ret = irq; > goto err_req_irq; > } > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/7] serial: clps711x: fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda ` (3 preceding siblings ...) 2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-15 14:35 ` [PATCH 6/7] fbdev: exynos: " Andrzej Hajda ` (2 subsequent siblings) 7 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Greg Kroah-Hartman, Jiri Slaby, Alexander Shiyan, linux-serial, linux-arm-kernel IS_ERR_VALUE macro should be used only with unsigned long type. For signed types comparison 'ret < 0' should be used. 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> --- drivers/tty/serial/clps711x.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c index b3a4e0c..0096ebe 100644 --- a/drivers/tty/serial/clps711x.c +++ b/drivers/tty/serial/clps711x.c @@ -467,13 +467,15 @@ static int uart_clps711x_probe(struct platform_device *pdev) if (IS_ERR(s->port.membase)) return PTR_ERR(s->port.membase); - s->port.irq = platform_get_irq(pdev, 0); - if (IS_ERR_VALUE(s->port.irq)) - return s->port.irq; + ret = platform_get_irq(pdev, 0); + if (ret < 0) + return ret; + s->port.irq = ret; - s->rx_irq = platform_get_irq(pdev, 1); - if (IS_ERR_VALUE(s->rx_irq)) - return s->rx_irq; + ret = platform_get_irq(pdev, 1); + if (ret < 0) + return ret; + s->rx_irq = ret; if (!np) { char syscon_name[9]; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] serial: clps711x: fix IS_ERR_VALUE usage 2016-02-15 14:35 ` [PATCH 5/7] serial: clps711x: " Andrzej Hajda @ 2016-02-17 2:33 ` Al Viro 0 siblings, 0 replies; 20+ messages in thread From: Al Viro @ 2016-02-17 2:33 UTC (permalink / raw) To: Andrzej Hajda Cc: linux-kernel, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Greg Kroah-Hartman, Jiri Slaby, Alexander Shiyan, linux-serial, linux-arm-kernel On Mon, Feb 15, 2016 at 03:35:23PM +0100, Andrzej Hajda wrote: > IS_ERR_VALUE macro should be used only with unsigned long type. > For signed types comparison 'ret < 0' should be used. ACK ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/7] fbdev: exynos: fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda ` (4 preceding siblings ...) 2016-02-15 14:35 ` [PATCH 5/7] serial: clps711x: " Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-16 13:36 ` Tomi Valkeinen 2016-02-15 14:35 ` [PATCH 7/7] usb: gadget: fsl_qe_udc: " Andrzej Hajda 2016-02-17 10:48 ` [PATCH 0/7] " Arnd Bergmann 7 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Inki Dae, Donghwa Lee, Kyungmin Park, Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev, linux-samsung-soc IS_ERR_VALUE macro should be used only with unsigned long type. For signed types comparison 'ret < 0' should be used. 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> --- drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c index b527fe4..951b592 100644 --- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c +++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c @@ -402,12 +402,12 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev) goto error; } - dsim->irq = platform_get_irq(pdev, 0); - if (IS_ERR_VALUE(dsim->irq)) { + ret = platform_get_irq(pdev, 0); + if (ret < 0) { dev_err(&pdev->dev, "failed to request dsim irq resource\n"); - ret = -EINVAL; goto error; } + dsim->irq = ret; init_completion(&dsim_wr_comp); init_completion(&dsim_rd_comp); -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 6/7] fbdev: exynos: fix IS_ERR_VALUE usage 2016-02-15 14:35 ` [PATCH 6/7] fbdev: exynos: " Andrzej Hajda @ 2016-02-16 13:36 ` Tomi Valkeinen 0 siblings, 0 replies; 20+ messages in thread From: Tomi Valkeinen @ 2016-02-16 13:36 UTC (permalink / raw) To: Andrzej Hajda, linux-kernel Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Inki Dae, Donghwa Lee, Kyungmin Park, Jean-Christophe Plagniol-Villard, linux-fbdev, linux-samsung-soc [-- Attachment #1: Type: text/plain, Size: 1308 bytes --] On 15/02/16 16:35, Andrzej Hajda wrote: > IS_ERR_VALUE macro should be used only with unsigned long type. > For signed types comparison 'ret < 0' should be used. > > 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> > --- > drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c > index b527fe4..951b592 100644 > --- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c > +++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c > @@ -402,12 +402,12 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev) > goto error; > } > > - dsim->irq = platform_get_irq(pdev, 0); > - if (IS_ERR_VALUE(dsim->irq)) { > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) { > dev_err(&pdev->dev, "failed to request dsim irq resource\n"); > - ret = -EINVAL; > goto error; > } > + dsim->irq = ret; > > init_completion(&dsim_wr_comp); > init_completion(&dsim_rd_comp); > Thanks, queued for 4.6. Tomi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/7] usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda ` (5 preceding siblings ...) 2016-02-15 14:35 ` [PATCH 6/7] fbdev: exynos: " Andrzej Hajda @ 2016-02-15 14:35 ` Andrzej Hajda 2016-02-17 10:48 ` [PATCH 0/7] " Arnd Bergmann 7 siblings, 0 replies; 20+ messages in thread From: Andrzej Hajda @ 2016-02-15 14:35 UTC (permalink / raw) To: linux-kernel Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, Li Yang, Felipe Balbi, Greg Kroah-Hartman, linux-usb, linuxppc-dev IS_ERR_VALUE macro should be used only with unsigned long type. Otherwise it can work incorrectly. 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> --- drivers/usb/gadget/udc/fsl_qe_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c index 53c0692..93d28cb 100644 --- a/drivers/usb/gadget/udc/fsl_qe_udc.c +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c @@ -2340,7 +2340,7 @@ static struct qe_udc *qe_udc_config(struct platform_device *ofdev) { struct qe_udc *udc; struct device_node *np = ofdev->dev.of_node; - unsigned int tmp_addr = 0; + unsigned long tmp_addr = 0; struct usb_device_para __iomem *usbpram; unsigned int i; u64 size; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 0/7] fix IS_ERR_VALUE usage 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda ` (6 preceding siblings ...) 2016-02-15 14:35 ` [PATCH 7/7] usb: gadget: fsl_qe_udc: " Andrzej Hajda @ 2016-02-17 10:48 ` Arnd Bergmann 7 siblings, 0 replies; 20+ messages in thread From: Arnd Bergmann @ 2016-02-17 10:48 UTC (permalink / raw) To: linux-arm-kernel Cc: Andrzej Hajda, linux-kernel, linux-mips, linux-fbdev, linux-samsung-soc, Bartlomiej Zolnierkiewicz, netdev, linux-usb, coreteam, netfilter-devel, linux-serial, Marek Szyprowski, linuxppc-dev, linux-media On Monday 15 February 2016 15:35:18 Andrzej Hajda wrote: > > Andrzej Hajda (7): > netfilter: fix IS_ERR_VALUE usage > MIPS: module: fix incorrect IS_ERR_VALUE macro usages > drivers: char: mem: fix IS_ERROR_VALUE usage > atmel-isi: fix IS_ERR_VALUE usage > serial: clps711x: fix IS_ERR_VALUE usage > fbdev: exynos: fix IS_ERR_VALUE usage > usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage > Can you Cc me the next time on all of the patches? I only got three of them this time. Arnd ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-02-21 17:51 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda 2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda 2016-02-17 2:31 ` Al Viro 2016-02-17 8:45 ` Andrzej Hajda 2016-02-17 12:41 ` [PATCH v2 " Andrzej Hajda 2016-02-17 13:42 ` Arnd Bergmann 2016-02-17 14:54 ` Andrzej Hajda 2016-02-17 15:40 ` Arnd Bergmann 2016-02-15 14:35 ` [PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages Andrzej Hajda 2016-02-15 14:35 ` [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-21 16:04 ` Guennadi Liakhovetski 2016-02-15 14:35 ` [PATCH 5/7] serial: clps711x: " Andrzej Hajda 2016-02-17 2:33 ` Al Viro 2016-02-15 14:35 ` [PATCH 6/7] fbdev: exynos: " Andrzej Hajda 2016-02-16 13:36 ` Tomi Valkeinen 2016-02-15 14:35 ` [PATCH 7/7] usb: gadget: fsl_qe_udc: " Andrzej Hajda 2016-02-17 10:48 ` [PATCH 0/7] " Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).