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

From: Arnd Bergmann
Date: Wed Feb 17 2016 - 10:41:04 EST


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