Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

[Arnd Bergmann]

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