Skip to contentSkip to conversation

netfilter: reduce hook sizes in struct net

PATCH In review Florian Westphal 2 participants 9 messages Nov 13, 2017 – Nov 22, 2017

Subsystems (3)
Networking netfilter Ethernet bridge
lore
Share & tools
lore

Florian Westphal

author maintainer
#0 lore raw

cover netfilter: reduce hook sizes in struct net

struct net contains:

struct nf_hook_entries __rcu *hooks[NFPROTO_NUMPROTO][NF_MAX_HOOKS];

which store the hook entry point locations for the various protocol families and the hooks.
This can be compacted a lot by only storing the families and hooks that are actually implemented.

struct net before:
/* size: 5184, cachelines: 81, members: 46 */
after:
/* size: 4544, cachelines: 71, members: 46 */

In case this is too late just ignore this thing, I will resubmit once next opens again.

author maintainer
#1 lore raw

PATCH 1/4 netfilter: reduce size of hook entry point locations

struct net contains:

struct nf_hook_entries __rcu *hooks[NFPROTO_NUMPROTO][NF_MAX_HOOKS];

which store the hook entry point locations for the various protocol families and the hooks.

Using array results in compact c code when doing accesses, i.e.
x = rcu_dereference(net->nf.hooks[pf][hook]);

but its also wasting a lot of memory, as most families are not used.

So split the array into those families that are used, which are only 5 (instead of 13). In most cases, the 'pf' argument is constant, i.e. gcc removes switch statement.

struct net before:
/* size: 5184, cachelines: 81, members: 46 */
after:
/* size: 4672, cachelines: 73, members: 46 */

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter.h       | 24 ++++++++++++++++++++++--
 include/net/netns/netfilter.h   |  6 +++++-
 net/bridge/br_netfilter_hooks.c |  2 +-
 net/netfilter/core.c            | 39 +++++++++++++++++++++++++++++++--------
 net/netfilter/nf_queue.c        | 19 ++++++++++++++++++-
 5 files changed, 77 insertions(+), 13 deletions(-)
include/linux/netfilter.h +22 -2
index b24e9b101651..80aa9a0b3d10 100644
@@ -184,7 +184,7 @@ static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 			  struct net_device *indev, struct net_device *outdev,
 			  int (*okfn)(struct net *, struct sock *, struct sk_buff *))
 {
-	struct nf_hook_entries *hook_head;
+	struct nf_hook_entries *hook_head = NULL;
 	int ret = 1;
 
 #ifdef HAVE_JUMP_LABEL

@@ -195,7 +195,27 @@ static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 #endif
 
 	rcu_read_lock();
-	hook_head = rcu_dereference(net->nf.hooks[pf][hook]);
+	switch (pf) {
+	case NFPROTO_IPV4:
+		hook_head = rcu_dereference(net->nf.hooks_ipv4[hook]);
+		break;
+	case NFPROTO_IPV6:
+		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
+		break;
+	case NFPROTO_ARP:
+		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
+		break;
+	case NFPROTO_BRIDGE:
+		hook_head = rcu_dereference(net->nf.hooks_bridge[hook]);
+		break;
+	case NFPROTO_DECNET:
+		hook_head = rcu_dereference(net->nf.hooks_decnet[hook]);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
+
 	if (hook_head) {
 		struct nf_hook_state state;
include/net/netns/netfilter.h +5 -1
index cc00af2ac2d7..b39c563c2fce 100644
@@ -17,7 +17,11 @@ struct netns_nf {
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header *nf_log_dir_header;
 #endif
-	struct nf_hook_entries __rcu *hooks[NFPROTO_NUMPROTO][NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_ipv4[NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_ipv6[NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_arp[NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_bridge[NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_decnet[NF_MAX_HOOKS];
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
 	bool			defrag_ipv4;
 #endif
net/bridge/br_netfilter_hooks.c +1 -1
index c2eea1b8737a..27f1d4f2114a 100644
@@ -991,7 +991,7 @@ int br_nf_hook_thresh(unsigned int hook, struct net *net,
 	unsigned int i;
 	int ret;
 
-	e = rcu_dereference(net->nf.hooks[NFPROTO_BRIDGE][hook]);
+	e = rcu_dereference(net->nf.hooks_bridge[hook]);
 	if (!e)
 		return okfn(net, sk, skb);
net/netfilter/core.c +31 -8
index 52cd2901a097..fd5f550dc625 100644
@@ -239,8 +239,23 @@ static void *__nf_hook_entries_try_shrink(struct nf_hook_entries __rcu **pp)
 
 static struct nf_hook_entries __rcu **nf_hook_entry_head(struct net *net, const struct nf_hook_ops *reg)
 {
-	if (reg->pf != NFPROTO_NETDEV)
-		return net->nf.hooks[reg->pf]+reg->hooknum;
+	switch (reg->pf) {
+	case NFPROTO_NETDEV:
+		break;
+	case NFPROTO_ARP:
+		return net->nf.hooks_arp+reg->hooknum;
+	case NFPROTO_BRIDGE:
+		return net->nf.hooks_bridge+reg->hooknum;
+	case NFPROTO_IPV4:
+		return net->nf.hooks_ipv4+reg->hooknum;
+	case NFPROTO_IPV6:
+		return net->nf.hooks_ipv6+reg->hooknum;
+	case NFPROTO_DECNET:
+		return net->nf.hooks_decnet+reg->hooknum;
+	default:
+		WARN_ON_ONCE(1);
+		return NULL;
+	}
 
 #ifdef CONFIG_NETFILTER_INGRESS
 	if (reg->hooknum == NF_NETDEV_INGRESS) {

@@ -569,14 +584,22 @@ void (*nf_nat_decode_session_hook)(struct sk_buff *, struct flowi *);
 EXPORT_SYMBOL(nf_nat_decode_session_hook);
 #endif
 
-static int __net_init netfilter_net_init(struct net *net)
+
+static void __net_init __netfilter_net_init(struct nf_hook_entries *e[NF_MAX_HOOKS])
 {
-	int i, h;
+	int h;
 
-	for (i = 0; i < ARRAY_SIZE(net->nf.hooks); i++) {
-		for (h = 0; h < NF_MAX_HOOKS; h++)
-			RCU_INIT_POINTER(net->nf.hooks[i][h], NULL);
-	}
+	for (h = 0; h < NF_MAX_HOOKS; h++)
+		RCU_INIT_POINTER(e[h], NULL);
+}
+
+static int __net_init netfilter_net_init(struct net *net)
+{
+	__netfilter_net_init(net->nf.hooks_ipv4);
+	__netfilter_net_init(net->nf.hooks_ipv6);
+	__netfilter_net_init(net->nf.hooks_arp);
+	__netfilter_net_init(net->nf.hooks_bridge);
+	__netfilter_net_init(net->nf.hooks_decnet);
 
 #ifdef CONFIG_PROC_FS
 	net->nf.proc_netfilter = proc_net_mkdir(net, "netfilter",
net/netfilter/nf_queue.c +18 -1
index f7e21953b1de..4fa97febf4e1 100644
@@ -204,6 +204,23 @@ static unsigned int nf_iterate(struct sk_buff *skb,
 	return NF_ACCEPT;
 }
 
+static struct nf_hook_entries *nf_hook_entries_head(const struct net *net, u8 pf, u8 hooknum)
+{
+	switch (pf) {
+	case NFPROTO_BRIDGE:
+		return rcu_dereference(net->nf.hooks_bridge[hooknum]);
+	case NFPROTO_IPV4:
+		return rcu_dereference(net->nf.hooks_ipv4[hooknum]);
+	case NFPROTO_IPV6:
+		return rcu_dereference(net->nf.hooks_ipv6[hooknum]);
+	default:
+		WARN_ON_ONCE(1);
+		return NULL;
+	}
+
+	return NULL;
+}
+
 /* Caller must hold rcu read-side lock */
 void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 {

@@ -219,7 +236,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	net = entry->state.net;
 	pf = entry->state.pf;
 
-	hooks = rcu_dereference(net->nf.hooks[pf][entry->state.hook]);
+	hooks = nf_hook_entries_head(net, pf, entry->state.hook);
 
 	nf_queue_entry_release_refs(entry);
Signature · 1 line
2.13.6
author maintainer
#2 lore raw

PATCH 2/4 netfilter: add BUILD_BUG_ON asserts for hook array sizes

In reply to Florian Westphal

Check that the array hooks are not accessed out-of-bounds. Next patch will then reduce their sizes to reflect the number of hooks implemented for each family.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter.h | 5 +++++
 1 file changed, 5 insertions(+)
include/linux/netfilter.h +5 -0
index 80aa9a0b3d10..2e9896d42f96 100644
@@ -197,18 +197,23 @@ static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 	rcu_read_lock();
 	switch (pf) {
 	case NFPROTO_IPV4:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_ipv4));
 		hook_head = rcu_dereference(net->nf.hooks_ipv4[hook]);
 		break;
 	case NFPROTO_IPV6:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_ipv6));
 		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
 		break;
 	case NFPROTO_ARP:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_arp));
 		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
 		break;
 	case NFPROTO_BRIDGE:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_bridge));
 		hook_head = rcu_dereference(net->nf.hooks_bridge[hook]);
 		break;
 	case NFPROTO_DECNET:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_decnet));
 		hook_head = rcu_dereference(net->nf.hooks_decnet[hook]);
 		break;
 	default:
Signature · 1 line
2.13.6
maintainer
#3 lore raw

Re: [PATCH nf-next 2/4] netfilter: add BUILD_BUG_ON asserts for hook array sizes

Hi Florian,

Quoted 3 lines · Florian Westphal

Check that the array hooks are not accessed out-of-bounds. Next patch will then reduce their sizes to reflect the number of hooks implemented for each family.

I'm hitting this here.

In file included from ./include/linux/kernel.h:10:0,

                 from ./include/linux/uio.h:12,
                 from ./include/linux/socket.h:8,
                 from net/decnet/dn_route.c:60:

In function ‘nf_hook.constprop’,
inlined from ‘NF_HOOK.constprop’ at
./include/linux/netfilter.h:279:6:
./include/linux/compiler.h:319:38: error: call to ‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

                                      ^
./include/linux/compiler.h:299:4: note: in definition of macro
‘__compiletime_assert’
    prefix ## suffix();    \
    ^

./include/linux/compiler.h:319:2: note: in expansion of macro ‘_compiletime_assert’
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ^
./include/linux/build_bug.h:47:37: note: in expansion of macro ‘compiletime_assert’

 #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                     ^

./include/linux/build_bug.h:71:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) ^
./include/linux/netfilter.h:221:3: note: in expansion of macro ‘BUILD_BUG_ON’
BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_decnet)); ^

author maintainer
#4 lore raw

Re: [PATCH nf-next 2/4] netfilter: add BUILD_BUG_ON asserts for hook array sizes

Quoted 13 lines · Pablo Neira Ayuso

Hi Florian,

Quoted 3 lines · Florian Westphal

Check that the array hooks are not accessed out-of-bounds. Next patch will then reduce their sizes to reflect the number of hooks implemented for each family.

I'm hitting this here.

In file included from ./include/linux/kernel.h:10:0,

                 from ./include/linux/uio.h:12,
                 from ./include/linux/socket.h:8,
                 from net/decnet/dn_route.c:60:
Quoted 7 lines

In function ‘nf_hook.constprop’,
inlined from ‘NF_HOOK.constprop’ at
./include/linux/netfilter.h:279:6:
./include/linux/compiler.h:319:38: error: call to ‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Thanks for the report, I guess gcc doesn't follow inline trail or argument isn't known at compile time.

I'll add a contant_p() test as well in v2.

maintainer
#5 lore raw

Re: [PATCH nf-next 2/4] netfilter: add BUILD_BUG_ON asserts for hook array sizes

Quoted 22 lines · Florian Westphal
Quoted 13 lines · Pablo Neira Ayuso

Hi Florian,

Quoted 3 lines · Florian Westphal

Check that the array hooks are not accessed out-of-bounds. Next patch will then reduce their sizes to reflect the number of hooks implemented for each family.

I'm hitting this here.

In file included from ./include/linux/kernel.h:10:0,

                 from ./include/linux/uio.h:12,
                 from ./include/linux/socket.h:8,
                 from net/decnet/dn_route.c:60:
Quoted 7 lines

In function ‘nf_hook.constprop’,
inlined from ‘NF_HOOK.constprop’ at
./include/linux/netfilter.h:279:6:
./include/linux/compiler.h:319:38: error: call to ‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Quoted 4 lines

Thanks for the report, I guess gcc doesn't follow inline trail or argument isn't known at compile time.

I'll add a contant_p() test as well in v2.

Side note: It's only happening with decnet.

author maintainer
#6 lore raw

PATCH 3/4 netfilter: reduce hook array sizes to what is needed

In reply to Florian Westphal

Not all families share the same hook count. Previous patch added build-time assert so reduce to what is needed.

Can't use the corresponding ARP, BRIDGE, DECNET defines because they sit in uapi headers and including them causes various build failures.

ARP BUILD_BUG_ON test now needs a 'builtin_constant_p' guard. A few call sites call nf_hook() with a non-compile-time-value in POST_ROUTING (which doesn't exist in ARP family) with.

In this case the BUILD_BUG_ON will trigger, so elide them there.

struct net before:
/* size: 4672, cachelines: 73, members: 46 */
after:
/* size: 4544, cachelines: 71, members: 46 */

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter.h     |  2 +-
 include/net/netns/netfilter.h | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)
include/linux/netfilter.h +1 -1
index 2e9896d42f96..48a2f0f93033 100644
@@ -205,7 +205,7 @@ static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
 		break;
 	case NFPROTO_ARP:
-		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_arp));
+		BUILD_BUG_ON(__builtin_constant_p(pf) && hook >= ARRAY_SIZE(net->nf.hooks_arp));
 		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
 		break;
 	case NFPROTO_BRIDGE:
include/net/netns/netfilter.h +5 -5
index b39c563c2fce..96b20b872353 100644
@@ -17,11 +17,11 @@ struct netns_nf {
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header *nf_log_dir_header;
 #endif
-	struct nf_hook_entries __rcu *hooks_ipv4[NF_MAX_HOOKS];
-	struct nf_hook_entries __rcu *hooks_ipv6[NF_MAX_HOOKS];
-	struct nf_hook_entries __rcu *hooks_arp[NF_MAX_HOOKS];
-	struct nf_hook_entries __rcu *hooks_bridge[NF_MAX_HOOKS];
-	struct nf_hook_entries __rcu *hooks_decnet[NF_MAX_HOOKS];
+	struct nf_hook_entries __rcu *hooks_ipv4[NF_INET_NUMHOOKS];
+	struct nf_hook_entries __rcu *hooks_ipv6[NF_INET_NUMHOOKS];
+	struct nf_hook_entries __rcu *hooks_arp[3];
+	struct nf_hook_entries __rcu *hooks_bridge[6];
+	struct nf_hook_entries __rcu *hooks_decnet[7];
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
 	bool			defrag_ipv4;
 #endif
Signature · 1 line
2.13.6
author maintainer
#7 lore raw

PATCH 4/4 netfilter: add ifdefs to avoid memory waste if family is not supported

In reply to Florian Westphal

No need to allocate space for families that are not supported in the kernel configuration.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter.h     |  6 ++++++
 include/net/netns/netfilter.h |  6 ++++++
 net/netfilter/core.c          | 12 ++++++++++++
 3 files changed, 24 insertions(+)
include/linux/netfilter.h +6 -0
index 48a2f0f93033..da03bfcc5084 100644
@@ -204,18 +204,24 @@ static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_ipv6));
 		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
 		break;
+#if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
 	case NFPROTO_ARP:
 		BUILD_BUG_ON(__builtin_constant_p(pf) && hook >= ARRAY_SIZE(net->nf.hooks_arp));
 		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
 		break;
+#endif
+#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
 	case NFPROTO_BRIDGE:
 		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_bridge));
 		hook_head = rcu_dereference(net->nf.hooks_bridge[hook]);
 		break;
+#endif
+#if IS_ENABLED(CONFIG_DECNET)
 	case NFPROTO_DECNET:
 		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_decnet));
 		hook_head = rcu_dereference(net->nf.hooks_decnet[hook]);
 		break;
+#endif
 	default:
 		WARN_ON_ONCE(1);
 		break;
include/net/netns/netfilter.h +6 -0
index 96b20b872353..2f9b445fe161 100644
@@ -19,9 +19,15 @@ struct netns_nf {
 #endif
 	struct nf_hook_entries __rcu *hooks_ipv4[NF_INET_NUMHOOKS];
 	struct nf_hook_entries __rcu *hooks_ipv6[NF_INET_NUMHOOKS];
+#if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
 	struct nf_hook_entries __rcu *hooks_arp[3];
+#endif
+#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
 	struct nf_hook_entries __rcu *hooks_bridge[6];
+#endif
+#if IS_ENABLED(CONFIG_DECNET)
 	struct nf_hook_entries __rcu *hooks_decnet[7];
+#endif
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
 	bool			defrag_ipv4;
 #endif
net/netfilter/core.c +12 -0
index fd5f550dc625..aeb7a4f8f080 100644
@@ -242,16 +242,22 @@ static struct nf_hook_entries __rcu **nf_hook_entry_head(struct net *net, const
 	switch (reg->pf) {
 	case NFPROTO_NETDEV:
 		break;
+#if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
 	case NFPROTO_ARP:
 		return net->nf.hooks_arp+reg->hooknum;
+#endif
+#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
 	case NFPROTO_BRIDGE:
 		return net->nf.hooks_bridge+reg->hooknum;
+#endif
 	case NFPROTO_IPV4:
 		return net->nf.hooks_ipv4+reg->hooknum;
 	case NFPROTO_IPV6:
 		return net->nf.hooks_ipv6+reg->hooknum;
+#if IS_ENABLED(CONFIG_DECNET)
 	case NFPROTO_DECNET:
 		return net->nf.hooks_decnet+reg->hooknum;
+#endif
 	default:
 		WARN_ON_ONCE(1);
 		return NULL;

@@ -597,9 +603,15 @@ static int __net_init netfilter_net_init(struct net *net)
 {
 	__netfilter_net_init(net->nf.hooks_ipv4);
 	__netfilter_net_init(net->nf.hooks_ipv6);
+#if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
 	__netfilter_net_init(net->nf.hooks_arp);
+#endif
+#if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
 	__netfilter_net_init(net->nf.hooks_bridge);
+#endif
+#if IS_ENABLED(CONFIG_DECNET)
 	__netfilter_net_init(net->nf.hooks_decnet);
+#endif
 
 #ifdef CONFIG_PROC_FS
 	net->nf.proc_netfilter = proc_net_mkdir(net, "netfilter",
Signature · 1 line
2.13.6
maintainer
#8 lore raw
In reply to Florian Westphal
Quoted 9 lines · Florian Westphal

struct net contains:

struct nf_hook_entries __rcu *hooks[NFPROTO_NUMPROTO][NF_MAX_HOOKS];

which store the hook entry point locations for the various protocol families and the hooks.
This can be compacted a lot by only storing the families and hooks that are actually implemented.

Quoted 7 lines

struct net before:
/* size: 5184, cachelines: 81, members: 46 */
after:
/* size: 4544, cachelines: 71, members: 46 */

In case this is too late just ignore this thing, I will resubmit once next opens again.

It's already closed, no problem, I'll let this sit into nf-next with other remaining patches I've been collecting today.