[PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[Florian Westphal]

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

Re: [PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[Leon Romanovsky]

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 *'.

<...>

> -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.

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>

Re: [PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[Florian Westphal]

Leon Romanovsky <leon@kernel.org> wrote:
> > xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
> > instead of 'void *'.
> 
> <...>
> 
> > -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.

Re: [PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[Leon Romanovsky]

On Tue, Feb 25, 2025 at 09:28:32AM +0100, Florian Westphal wrote:
> Leon Romanovsky <leon@kernel.org> wrote:
> > > xfrm_hash_free() is kept around because of 'struct hlist_head *' arg type
> > > instead of 'void *'.
> > 
> > <...>
> > 
> > > -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.
> 

Re: [PATCH ipsec-next] xfrm: remove hash table alloc/free helpers

[kernel test robot]

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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502260536.BzRJ7jGr-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> 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);
e88add19f68191 Ahmed S. Darwish 2021-03-16  193  	write_seqcount_begin(&net->xfrm.xfrm_state_hash_generation);
f034b5d4efdfe0 David S. Miller  2006-08-24  194  
f034b5d4efdfe0 David S. Miller  2006-08-24  195  	nhashmask = (nsize / sizeof(struct hlist_head)) - 1U;
c8406998b80183 Florian Westphal 2016-08-09  196  	odst = xfrm_state_deref_prot(net->xfrm.state_bydst, net);
63082733858502 Alexey Dobriyan  2008-11-25  197  	for (i = net->xfrm.state_hmask; i >= 0; i--)
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  198  		xfrm_hash_transfer(odst + i, ndst, nsrc, nspi, nseq, nhashmask);
f034b5d4efdfe0 David S. Miller  2006-08-24  199  
c8406998b80183 Florian Westphal 2016-08-09  200  	osrc = xfrm_state_deref_prot(net->xfrm.state_bysrc, net);
c8406998b80183 Florian Westphal 2016-08-09  201  	ospi = xfrm_state_deref_prot(net->xfrm.state_byspi, net);
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  202  	oseq = xfrm_state_deref_prot(net->xfrm.state_byseq, net);
63082733858502 Alexey Dobriyan  2008-11-25  203  	ohashmask = net->xfrm.state_hmask;
f034b5d4efdfe0 David S. Miller  2006-08-24  204  
c8406998b80183 Florian Westphal 2016-08-09  205  	rcu_assign_pointer(net->xfrm.state_bydst, ndst);
c8406998b80183 Florian Westphal 2016-08-09  206  	rcu_assign_pointer(net->xfrm.state_bysrc, nsrc);
c8406998b80183 Florian Westphal 2016-08-09  207  	rcu_assign_pointer(net->xfrm.state_byspi, nspi);
fe9f1d8779cb47 Sabrina Dubroca  2021-04-25  208  	rcu_assign_pointer(net->xfrm.state_byseq, nseq);
63082733858502 Alexey Dobriyan  2008-11-25  209  	net->xfrm.state_hmask = nhashmask;
f034b5d4efdfe0 David S. Miller  2006-08-24  210  
e88add19f68191 Ahmed S. Darwish 2021-03-16  211  	write_seqcount_end(&net->xfrm.xfrm_state_hash_generation);
283bc9f35bbbcb Fan Du           2013-11-07  212  	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
f034b5d4efdfe0 David S. Miller  2006-08-24  213  
df7274eb70b7c8 Florian Westphal 2016-08-09  214  	synchronize_rcu();
df7274eb70b7c8 Florian Westphal 2016-08-09  215  
ef6562fb0dcb1a Florian Westphal 2025-02-24  216  	xfrm_hash_free(odst);
ef6562fb0dcb1a Florian Westphal 2025-02-24  217  	xfrm_hash_free(osrc);
ef6562fb0dcb1a Florian Westphal 2025-02-24  218  	xfrm_hash_free(ospi);
ef6562fb0dcb1a Florian Westphal 2025-02-24  219  	xfrm_hash_free(oseq);
f034b5d4efdfe0 David S. Miller  2006-08-24  220  }
f034b5d4efdfe0 David S. Miller  2006-08-24  221  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki