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