Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage | Date | Wed, 17 Feb 2016 14:42:16 +0100 |
| |
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
| |