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

[Andrzej Hajda]

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