Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1336530

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

From Arnd Bergmann <arnd@arndb.de>
Newsgroups linux.kernel
Subject Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage
Date 2016-02-17 16:50 +0100
Message-ID <r3cwx-8pl-5@gated-at.bofh.it> (permalink)
References <r30c2-8ce-17@gated-at.bofh.it> <r3aEs-770-35@gated-at.bofh.it> <r3bKa-7NK-33@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw


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

Back to linux.kernel | Previous | NextPrevious in thread | Find similar | Unroll thread


Thread

[PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-15 15:40 +0100
  Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-17 03:40 +0100
    Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 09:50 +0100
    [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 13:50 +0100
      Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Arnd Bergmann <arnd@arndb.de> - 2016-02-17 14:50 +0100
        Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Andrzej Hajda <a.hajda@samsung.com> - 2016-02-17 16:00 +0100
          Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Arnd Bergmann <arnd@arndb.de> - 2016-02-17 16:50 +0100

csiph-web