xfrm: remove hash table alloc/free helpers

These functions predate kvmalloc, update xfrm to use that instead.
This also allows to drop the 'size' argument passed to xfrm_hash_free().

xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
instead of 'void *'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/xfrm/Makefile      |  2 +-
 net/xfrm/xfrm_hash.c   | 40 ---------------------------------------
 net/xfrm/xfrm_hash.h   | 10 ++++++++--
 net/xfrm/xfrm_policy.c | 15 ++++++---------
 net/xfrm/xfrm_state.c  | 43 +++++++++++++++++++-----------------------
 5 files changed, 34 insertions(+), 76 deletions(-)
 delete mode 100644 net/xfrm/xfrm_hash.c

diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
index 5a1787587cb3..38a710684848 100644
--- a/net/xfrm/Makefile
+++ b/net/xfrm/Makefile

@@ -11,7 +11,7 @@ else ifeq ($(CONFIG_XFRM_INTERFACE),y)
 xfrm_interface-$(CONFIG_DEBUG_INFO_BTF) += xfrm_interface_bpf.o
 endif
 
-obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
+obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o \
 		      xfrm_input.o xfrm_output.o \
 		      xfrm_sysctl.o xfrm_replay.o xfrm_device.o \
 		      xfrm_nat_keepalive.o

diff --git a/net/xfrm/xfrm_hash.c b/net/xfrm/xfrm_hash.c
deleted file mode 100644
index eca8d84d99bf..000000000000
--- a/net/xfrm/xfrm_hash.c
+++ /dev/null

@@ -1,40 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* xfrm_hash.c: Common hash table code.
- *
- * Copyright (C) 2006 David S. Miller (davem@davemloft.net)
- */
-
-#include <linux/kernel.h>
-#include <linux/mm.h>
-#include <linux/memblock.h>
-#include <linux/vmalloc.h>
-#include <linux/slab.h>
-#include <linux/xfrm.h>
-
-#include "xfrm_hash.h"
-
-struct hlist_head *xfrm_hash_alloc(unsigned int sz)
-{
-	struct hlist_head *n;
-
-	if (sz <= PAGE_SIZE)
-		n = kzalloc(sz, GFP_KERNEL);
-	else if (hashdist)
-		n = vzalloc(sz);
-	else
-		n = (struct hlist_head *)
-			__get_free_pages(GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO,
-					 get_order(sz));
-
-	return n;
-}
-
-void xfrm_hash_free(struct hlist_head *n, unsigned int sz)
-{
-	if (sz <= PAGE_SIZE)
-		kfree(n);
-	else if (hashdist)
-		vfree(n);
-	else
-		free_pages((unsigned long)n, get_order(sz));
-}

diff --git a/net/xfrm/xfrm_hash.h b/net/xfrm/xfrm_hash.h
index d12bb906c9c9..0a91b4d84fda 100644
--- a/net/xfrm/xfrm_hash.h
+++ b/net/xfrm/xfrm_hash.h

@@ -193,7 +193,13 @@ static inline unsigned int __addr_hash(const xfrm_address_t *daddr,
 	return h & hmask;
 }
 
-struct hlist_head *xfrm_hash_alloc(unsigned int sz);
-void xfrm_hash_free(struct hlist_head *n, unsigned int sz);
+static inline struct hlist_head *xfrm_hash_alloc(unsigned int sz)
+{
+	return kvzalloc(sz, GFP_KERNEL);
+}
 
+static inline void xfrm_hash_free(struct hlist_head *n)
+{
+	kvfree(n);
+}
 #endif /* _XFRM_HASH_H */

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 6551e588fe52..e75be29865ff 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c

@@ -655,7 +655,7 @@ static void xfrm_bydst_resize(struct net *net, int dir)
 
 	synchronize_rcu();
 
-	xfrm_hash_free(odst, (hmask + 1) * sizeof(struct hlist_head));
+	xfrm_hash_free(odst);
 }
 
 static void xfrm_byidx_resize(struct net *net)

@@ -680,7 +680,7 @@ static void xfrm_byidx_resize(struct net *net)
 
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 
-	xfrm_hash_free(oidx, (hmask + 1) * sizeof(struct hlist_head));
+	xfrm_hash_free(oidx);
 }
 
 static inline int xfrm_bydst_should_resize(struct net *net, int dir, int *total)

@@ -4253,9 +4253,9 @@ static int __net_init xfrm_policy_init(struct net *net)
 		struct xfrm_policy_hash *htab;
 
 		htab = &net->xfrm.policy_bydst[dir];
-		xfrm_hash_free(htab->table, sz);
+		xfrm_hash_free(htab->table);
 	}
-	xfrm_hash_free(net->xfrm.policy_byidx, sz);
+	xfrm_hash_free(net->xfrm.policy_byidx);
 out_byidx:
 	return -ENOMEM;
 }

@@ -4263,7 +4263,6 @@ static int __net_init xfrm_policy_init(struct net *net)
 static void xfrm_policy_fini(struct net *net)
 {
 	struct xfrm_pol_inexact_bin *b, *t;
-	unsigned int sz;
 	int dir;
 
 	flush_work(&net->xfrm.policy_hash_work);

@@ -4278,14 +4277,12 @@ static void xfrm_policy_fini(struct net *net)
 		struct xfrm_policy_hash *htab;
 
 		htab = &net->xfrm.policy_bydst[dir];
-		sz = (htab->hmask + 1) * sizeof(struct hlist_head);
 		WARN_ON(!hlist_empty(htab->table));
-		xfrm_hash_free(htab->table, sz);
+		xfrm_hash_free(htab->table);
 	}
 
-	sz = (net->xfrm.policy_idx_hmask + 1) * sizeof(struct hlist_head);
 	WARN_ON(!hlist_empty(net->xfrm.policy_byidx));
-	xfrm_hash_free(net->xfrm.policy_byidx, sz);
+	xfrm_hash_free(net->xfrm.policy_byidx);
 
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
 	list_for_each_entry_safe(b, t, &net->xfrm.inexact_bins, inexact_bins)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index ad2202fa82f3..5efe7af68f1a 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -162,8 +162,8 @@ static void xfrm_hash_resize(struct work_struct *work)
 {
 	struct net *net = container_of(work, struct net, xfrm.state_hash_work);
 	struct hlist_head *ndst, *nsrc, *nspi, *nseq, *odst, *osrc, *ospi, *oseq;
-	unsigned long nsize, osize;
 	unsigned int nhashmask, ohashmask;
+	unsigned long nsize;
 	int i;
 
 	nsize = xfrm_hash_new_size(net->xfrm.state_hmask);

@@ -172,20 +172,20 @@ static void xfrm_hash_resize(struct work_struct *work)
 		return;
 	nsrc = xfrm_hash_alloc(nsize);
 	if (!nsrc) {
-		xfrm_hash_free(ndst, nsize);
+		xfrm_hash_free(ndst);
 		return;
 	}
 	nspi = xfrm_hash_alloc(nsize);
 	if (!nspi) {
-		xfrm_hash_free(ndst, nsize);
-		xfrm_hash_free(nsrc, nsize);
+		xfrm_hash_free(ndst);
+		xfrm_hash_free(nsrc);
 		return;
 	}
 	nseq = xfrm_hash_alloc(nsize);
 	if (!nseq) {
-		xfrm_hash_free(ndst, nsize);
-		xfrm_hash_free(nsrc, nsize);
-		xfrm_hash_free(nspi, nsize);
+		xfrm_hash_free(ndst);
+		xfrm_hash_free(nsrc);
+		xfrm_hash_free(nspi);
 		return;
 	}
 

@@ -211,14 +211,12 @@ static void xfrm_hash_resize(struct work_struct *work)
 	write_seqcount_end(&net->xfrm.xfrm_state_hash_generation);
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
-	osize = (ohashmask + 1) * sizeof(struct hlist_head);
-
 	synchronize_rcu();
 
-	xfrm_hash_free(odst, osize);
-	xfrm_hash_free(osrc, osize);
-	xfrm_hash_free(ospi, osize);
-	xfrm_hash_free(oseq, osize);
+	xfrm_hash_free(odst);
+	xfrm_hash_free(osrc);
+	xfrm_hash_free(ospi);
+	xfrm_hash_free(oseq);
 }
 
 static DEFINE_SPINLOCK(xfrm_state_afinfo_lock);

@@ -3277,36 +3275,33 @@ int __net_init xfrm_state_init(struct net *net)
 	return 0;
 
 out_state_cache_input:
-	xfrm_hash_free(net->xfrm.state_byseq, sz);
+	xfrm_hash_free(net->xfrm.state_byseq);
 out_byseq:
-	xfrm_hash_free(net->xfrm.state_byspi, sz);
+	xfrm_hash_free(net->xfrm.state_byspi);
 out_byspi:
-	xfrm_hash_free(net->xfrm.state_bysrc, sz);
+	xfrm_hash_free(net->xfrm.state_bysrc);
 out_bysrc:
-	xfrm_hash_free(net->xfrm.state_bydst, sz);
+	xfrm_hash_free(net->xfrm.state_bydst);
 out_bydst:
 	return -ENOMEM;
 }
 
 void xfrm_state_fini(struct net *net)
 {
-	unsigned int sz;
-
 	flush_work(&net->xfrm.state_hash_work);
 	flush_work(&xfrm_state_gc_work);
 	xfrm_state_flush(net, 0, false, true);
 
 	WARN_ON(!list_empty(&net->xfrm.state_all));
 
-	sz = (net->xfrm.state_hmask + 1) * sizeof(struct hlist_head);
 	WARN_ON(!hlist_empty(net->xfrm.state_byseq));
-	xfrm_hash_free(net->xfrm.state_byseq, sz);
+	xfrm_hash_free(net->xfrm.state_byseq);
 	WARN_ON(!hlist_empty(net->xfrm.state_byspi));
-	xfrm_hash_free(net->xfrm.state_byspi, sz);
+	xfrm_hash_free(net->xfrm.state_byspi);
 	WARN_ON(!hlist_empty(net->xfrm.state_bysrc));
-	xfrm_hash_free(net->xfrm.state_bysrc, sz);
+	xfrm_hash_free(net->xfrm.state_bysrc);
 	WARN_ON(!hlist_empty(net->xfrm.state_bydst));
-	xfrm_hash_free(net->xfrm.state_bydst, sz);
+	xfrm_hash_free(net->xfrm.state_bydst);
 	free_percpu(net->xfrm.state_cache_input);
 }
 

-- 
2.45.3

On Mon, Feb 24, 2025 at 06:10:50PM +0100, Florian Westphal wrote:

These functions predate kvmalloc, update xfrm to use that instead.
This also allows to drop the 'size' argument passed to xfrm_hash_free().

xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
instead of 'void *'.

<...>

Sorry, what does this wrapper give us?
You are passing pointer as is and there is no any pointer type check
that this construction will give us.

I would say that there is no need to hide basic kernel primitives
like kvzalloc and kvfree, and better leave them as is without wrappers.

The change itself looks good,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Leon Romanovsky [off-list ref] wrote:

quoted

xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
instead of 'void *'.

<...>

quoted

-struct hlist_head *xfrm_hash_alloc(unsigned int sz);
-void xfrm_hash_free(struct hlist_head *n, unsigned int sz);
+static inline struct hlist_head *xfrm_hash_alloc(unsigned int sz)
+{
+	return kvzalloc(sz, GFP_KERNEL);
+}
 
+static inline void xfrm_hash_free(struct hlist_head *n)
+{
+	kvfree(n);
+}

Sorry, what does this wrapper give us?
You are passing pointer as is and there is no any pointer type check
that this construction will give us.

Compiler will warn when the argument is something other than a pointer
to a hlist_head.

I can send a v2 with this wrapper removed if you don't think its worth it.

Thanks for reviewing.

On Tue, Feb 25, 2025 at 09:28:32AM +0100, Florian Westphal wrote:

Leon Romanovsky [off-list ref] wrote:

quoted

quoted

xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
instead of 'void *'.

<...>

quoted

-struct hlist_head *xfrm_hash_alloc(unsigned int sz);
-void xfrm_hash_free(struct hlist_head *n, unsigned int sz);
+static inline struct hlist_head *xfrm_hash_alloc(unsigned int sz)
+{
+	return kvzalloc(sz, GFP_KERNEL);
+}
 
+static inline void xfrm_hash_free(struct hlist_head *n)
+{
+	kvfree(n);
+}

Sorry, what does this wrapper give us?
You are passing pointer as is and there is no any pointer type check
that this construction will give us.

Compiler will warn when the argument is something other than a pointer
to a hlist_head.

I personally didn't see any bug where wrong pointer was passed to kfree :).

I can send a v2 with this wrapper removed if you don't think its worth it.

It is up to you.

Thanks for reviewing.

Hi Florian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on klassert-ipsec-next/master]
[also build test WARNING on klassert-ipsec/master net/main net-next/main linus/master v6.14-rc4 next-20250225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Westphal/xfrm-remove-hash-table-alloc-free-helpers/20250225-011758
base:   https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
patch link:    https://lore.kernel.org/r/20250224171055.15951-1-fw%40strlen.de
patch subject: [PATCH ipsec-next] xfrm: remove hash table alloc/free helpers
config: x86_64-buildonly-randconfig-004-20250226 (https://download.01.org/0day-ci/archive/20250226/202502260536.BzRJ7jGr-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250226/202502260536.BzRJ7jGr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot [off-list ref]
| Closes: https://lore.kernel.org/oe-kbuild-all/202502260536.BzRJ7jGr-lkp@intel.com/

All warnings (new ones prefixed by >>):

quoted

net/xfrm/xfrm_state.c:165:26: warning: variable 'ohashmask' set but not used [-Wunused-but-set-variable]

     165 |         unsigned int nhashmask, ohashmask;
         |                                 ^
   1 warning generated.


vim +/ohashmask +165 net/xfrm/xfrm_state.c

f034b5d4efdfe0 David S. Miller  2006-08-24  160  
63082733858502 Alexey Dobriyan  2008-11-25  161  static void xfrm_hash_resize(struct work_struct *work)
f034b5d4efdfe0 David S. Miller  2006-08-24  162  {
63082733858502 Alexey Dobriyan  2008-11-25  163  	struct net *net = container_of(work, struct net, xfrm.state_hash_work);
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  164  	struct hlist_head *ndst, *nsrc, *nspi, *nseq, *odst, *osrc, *ospi, *oseq;
f034b5d4efdfe0 David S. Miller  2006-08-24 @165  	unsigned int nhashmask, ohashmask;
ef6562fb0dcb1a Florian Westphal 2025-02-24  166  	unsigned long nsize;
f034b5d4efdfe0 David S. Miller  2006-08-24  167  	int i;
f034b5d4efdfe0 David S. Miller  2006-08-24  168  
63082733858502 Alexey Dobriyan  2008-11-25  169  	nsize = xfrm_hash_new_size(net->xfrm.state_hmask);
44e36b42a8378b David S. Miller  2006-08-24  170  	ndst = xfrm_hash_alloc(nsize);
f034b5d4efdfe0 David S. Miller  2006-08-24  171  	if (!ndst)
0244790c8ad240 Ying Xue         2014-08-29  172  		return;
44e36b42a8378b David S. Miller  2006-08-24  173  	nsrc = xfrm_hash_alloc(nsize);
f034b5d4efdfe0 David S. Miller  2006-08-24  174  	if (!nsrc) {
ef6562fb0dcb1a Florian Westphal 2025-02-24  175  		xfrm_hash_free(ndst);
0244790c8ad240 Ying Xue         2014-08-29  176  		return;
f034b5d4efdfe0 David S. Miller  2006-08-24  177  	}
44e36b42a8378b David S. Miller  2006-08-24  178  	nspi = xfrm_hash_alloc(nsize);
f034b5d4efdfe0 David S. Miller  2006-08-24  179  	if (!nspi) {
ef6562fb0dcb1a Florian Westphal 2025-02-24  180  		xfrm_hash_free(ndst);
ef6562fb0dcb1a Florian Westphal 2025-02-24  181  		xfrm_hash_free(nsrc);
0244790c8ad240 Ying Xue         2014-08-29  182  		return;
f034b5d4efdfe0 David S. Miller  2006-08-24  183  	}
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  184  	nseq = xfrm_hash_alloc(nsize);
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  185  	if (!nseq) {
ef6562fb0dcb1a Florian Westphal 2025-02-24  186  		xfrm_hash_free(ndst);
ef6562fb0dcb1a Florian Westphal 2025-02-24  187  		xfrm_hash_free(nsrc);
ef6562fb0dcb1a Florian Westphal 2025-02-24  188  		xfrm_hash_free(nspi);
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  189  		return;
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  190  	}
f034b5d4efdfe0 David S. Miller  2006-08-24  191  
283bc9f35bbbcb Fan Du           2013-11-07  192  	spin_lock_bh(&net->xfrm.xfrm_state_lock);

33 more lines