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
