[PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.

[PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.

[Steffen Klassert]

When offloading xfrm states to hardware, the offloading
device is attached to the skbs secpath. If a skb is free
is deffered, an unregister netdevice hangs because the
netdevice is still refcounted.

Fix this by removing the netdevice from the xfrm states
when the netdevice is unregisterd. To find all xfrm states
that need to be cleared we add another list where skbs
linked to that are unlinked from the lists (deleted)
but not yet freed.

Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Tested-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/xfrm.h    | 36 ++++++++------------------
 net/xfrm/xfrm_state.c | 59 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 67 insertions(+), 28 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 77ebf5bcf0b9..7d4c2235252c 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -178,7 +178,10 @@ struct xfrm_state {
 		struct hlist_node	gclist;
 		struct hlist_node	bydst;
 	};
-	struct hlist_node	bysrc;
+	union {
+		struct hlist_node	dev_gclist;
+		struct hlist_node	bysrc;
+	};
 	struct hlist_node	byspi;
 	struct hlist_node	byseq;
 
@@ -1588,7 +1591,7 @@ void xfrm_state_update_stats(struct net *net);
 static inline void xfrm_dev_state_update_stats(struct xfrm_state *x)
 {
 	struct xfrm_dev_offload *xdo = &x->xso;
-	struct net_device *dev = xdo->dev;
+	struct net_device *dev = READ_ONCE(xdo->dev);
 
 	if (dev && dev->xfrmdev_ops &&
 	    dev->xfrmdev_ops->xdo_dev_state_update_stats)
@@ -1946,13 +1949,16 @@ int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
 			struct xfrm_user_offload *xuo, u8 dir,
 			struct netlink_ext_ack *extack);
 bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
+void xfrm_dev_state_delete(struct xfrm_state *x);
+void xfrm_dev_state_free(struct xfrm_state *x);
 
 static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
 {
 	struct xfrm_dev_offload *xso = &x->xso;
+	struct net_device *dev = READ_ONCE(xso->dev);
 
-	if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
-		xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
+	if (dev && dev->xfrmdev_ops->xdo_dev_state_advance_esn)
+		dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
 }
 
 static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
@@ -1973,28 +1979,6 @@ static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
 	return false;
 }
 
-static inline void xfrm_dev_state_delete(struct xfrm_state *x)
-{
-	struct xfrm_dev_offload *xso = &x->xso;
-
-	if (xso->dev)
-		xso->dev->xfrmdev_ops->xdo_dev_state_delete(x);
-}
-
-static inline void xfrm_dev_state_free(struct xfrm_state *x)
-{
-	struct xfrm_dev_offload *xso = &x->xso;
-	struct net_device *dev = xso->dev;
-
-	if (dev && dev->xfrmdev_ops) {
-		if (dev->xfrmdev_ops->xdo_dev_state_free)
-			dev->xfrmdev_ops->xdo_dev_state_free(x);
-		xso->dev = NULL;
-		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
-		netdev_put(dev, &xso->dev_tracker);
-	}
-}
-
 static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
 {
 	struct xfrm_dev_offload *xdo = &x->xdo;
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 649bb739df0d..816dda229e45 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -49,6 +49,7 @@ static struct kmem_cache *xfrm_state_cache __ro_after_init;
 
 static DECLARE_WORK(xfrm_state_gc_work, xfrm_state_gc_task);
 static HLIST_HEAD(xfrm_state_gc_list);
+static HLIST_HEAD(xfrm_state_dev_gc_list);
 
 static inline bool xfrm_state_hold_rcu(struct xfrm_state __rcu *x)
 {
@@ -214,6 +215,7 @@ static DEFINE_SPINLOCK(xfrm_state_afinfo_lock);
 static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];
 
 static DEFINE_SPINLOCK(xfrm_state_gc_lock);
+static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
 
 int __xfrm_state_delete(struct xfrm_state *x);
 
@@ -683,6 +685,38 @@ struct xfrm_state *xfrm_state_alloc(struct net *net)
 }
 EXPORT_SYMBOL(xfrm_state_alloc);
 
+void xfrm_dev_state_delete(struct xfrm_state *x)
+{
+	struct xfrm_dev_offload *xso = &x->xso;
+	struct net_device *dev = READ_ONCE(xso->dev);
+
+	if (dev) {
+		dev->xfrmdev_ops->xdo_dev_state_delete(x);
+		spin_lock_bh(&xfrm_state_dev_gc_lock);
+		hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
+		spin_unlock_bh(&xfrm_state_dev_gc_lock);
+	}
+}
+
+void xfrm_dev_state_free(struct xfrm_state *x)
+{
+	struct xfrm_dev_offload *xso = &x->xso;
+	struct net_device *dev = READ_ONCE(xso->dev);
+
+	if (dev && dev->xfrmdev_ops) {
+		spin_lock_bh(&xfrm_state_dev_gc_lock);
+		if (!hlist_unhashed(&x->dev_gclist))
+			hlist_del(&x->dev_gclist);
+		spin_unlock_bh(&xfrm_state_dev_gc_lock);
+
+		if (dev->xfrmdev_ops->xdo_dev_state_free)
+			dev->xfrmdev_ops->xdo_dev_state_free(x);
+		WRITE_ONCE(xso->dev, NULL);
+		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
+		netdev_put(dev, &xso->dev_tracker);
+	}
+}
+
 void __xfrm_state_destroy(struct xfrm_state *x, bool sync)
 {
 	WARN_ON(x->km.state != XFRM_STATE_DEAD);
@@ -848,6 +882,9 @@ EXPORT_SYMBOL(xfrm_state_flush);
 
 int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_valid)
 {
+	struct xfrm_state *x;
+	struct hlist_node *tmp;
+	struct xfrm_dev_offload *xso;
 	int i, err = 0, cnt = 0;
 
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
@@ -857,8 +894,6 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 
 	err = -ESRCH;
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
-		struct xfrm_state *x;
-		struct xfrm_dev_offload *xso;
 restart:
 		hlist_for_each_entry(x, net->xfrm.state_bydst+i, bydst) {
 			xso = &x->xso;
@@ -868,6 +903,8 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 				spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
 				err = xfrm_state_delete(x);
+				xfrm_dev_state_free(x);
+
 				xfrm_audit_state_delete(x, err ? 0 : 1,
 							task_valid);
 				xfrm_state_put(x);
@@ -884,6 +921,24 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 
 out:
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
+
+	spin_lock_bh(&xfrm_state_dev_gc_lock);
+restart_gc:
+	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
+		xso = &x->xso;
+
+		if (xso->dev == dev) {
+			spin_unlock_bh(&xfrm_state_dev_gc_lock);
+			xfrm_dev_state_free(x);
+			spin_lock_bh(&xfrm_state_dev_gc_lock);
+			goto restart_gc;
+		}
+
+	}
+	spin_unlock_bh(&xfrm_state_dev_gc_lock);
+
+	xfrm_flush_gc();
+
 	return err;
 }
 EXPORT_SYMBOL(xfrm_dev_state_flush);
-- 
2.34.1

Re: [PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.

[kernel test robot]

Hi Steffen,

kernel test robot noticed the following build errors:

[auto build test ERROR on klassert-ipsec-next/master]
[also build test ERROR on klassert-ipsec/master net/main net-next/main linus/master v6.10-rc3 next-20240612]
[cannot apply to horms-ipvs/master]
[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/Steffen-Klassert/xfrm-Fix-unregister-netdevice-hang-on-hardware-offload/20240612-171414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
patch link:    https://lore.kernel.org/r/ZmlmTTYL6AkBel4P%40gauss3.secunet.de
patch subject: [PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20240612/202406122125.fzbXlLhC-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406122125.fzbXlLhC-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/202406122125.fzbXlLhC-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/xfrm/xfrm_state.c:688:6: error: redefinition of 'xfrm_dev_state_delete'
     688 | void xfrm_dev_state_delete(struct xfrm_state *x)
         |      ^~~~~~~~~~~~~~~~~~~~~
   In file included from net/xfrm/xfrm_state.c:19:
   include/net/xfrm.h:2022:20: note: previous definition of 'xfrm_dev_state_delete' with type 'void(struct xfrm_state *)'
    2022 | static inline void xfrm_dev_state_delete(struct xfrm_state *x)
         |                    ^~~~~~~~~~~~~~~~~~~~~
   net/xfrm/xfrm_state.c: In function 'xfrm_dev_state_delete':
>> net/xfrm/xfrm_state.c:694:20: error: 'struct net_device' has no member named 'xfrmdev_ops'
     694 |                 dev->xfrmdev_ops->xdo_dev_state_delete(x);
         |                    ^~
   net/xfrm/xfrm_state.c: At top level:
>> net/xfrm/xfrm_state.c:701:6: error: redefinition of 'xfrm_dev_state_free'
     701 | void xfrm_dev_state_free(struct xfrm_state *x)
         |      ^~~~~~~~~~~~~~~~~~~
   include/net/xfrm.h:2026:20: note: previous definition of 'xfrm_dev_state_free' with type 'void(struct xfrm_state *)'
    2026 | static inline void xfrm_dev_state_free(struct xfrm_state *x)
         |                    ^~~~~~~~~~~~~~~~~~~
   net/xfrm/xfrm_state.c: In function 'xfrm_dev_state_free':
   net/xfrm/xfrm_state.c:706:23: error: 'struct net_device' has no member named 'xfrmdev_ops'
     706 |         if (dev && dev->xfrmdev_ops) {
         |                       ^~
   net/xfrm/xfrm_state.c:712:24: error: 'struct net_device' has no member named 'xfrmdev_ops'
     712 |                 if (dev->xfrmdev_ops->xdo_dev_state_free)
         |                        ^~
   net/xfrm/xfrm_state.c:713:28: error: 'struct net_device' has no member named 'xfrmdev_ops'
     713 |                         dev->xfrmdev_ops->xdo_dev_state_free(x);
         |                            ^~


vim +/xfrm_dev_state_delete +688 net/xfrm/xfrm_state.c

   687	
 > 688	void xfrm_dev_state_delete(struct xfrm_state *x)
   689	{
   690		struct xfrm_dev_offload *xso = &x->xso;
   691		struct net_device *dev = READ_ONCE(xso->dev);
   692	
   693		if (dev) {
 > 694			dev->xfrmdev_ops->xdo_dev_state_delete(x);
   695			spin_lock_bh(&xfrm_state_dev_gc_lock);
   696			hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
   697			spin_unlock_bh(&xfrm_state_dev_gc_lock);
   698		}
   699	}
   700	
 > 701	void xfrm_dev_state_free(struct xfrm_state *x)
   702	{
   703		struct xfrm_dev_offload *xso = &x->xso;
   704		struct net_device *dev = READ_ONCE(xso->dev);
   705	
   706		if (dev && dev->xfrmdev_ops) {
   707			spin_lock_bh(&xfrm_state_dev_gc_lock);
   708			if (!hlist_unhashed(&x->dev_gclist))
   709				hlist_del(&x->dev_gclist);
   710			spin_unlock_bh(&xfrm_state_dev_gc_lock);
   711	
   712			if (dev->xfrmdev_ops->xdo_dev_state_free)
   713				dev->xfrmdev_ops->xdo_dev_state_free(x);
   714			WRITE_ONCE(xso->dev, NULL);
   715			xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
   716			netdev_put(dev, &xso->dev_tracker);
   717		}
   718	}
   719	

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

Re: [PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.

[kernel test robot]

Hi Steffen,

kernel test robot noticed the following build errors:

[auto build test ERROR on klassert-ipsec-next/master]
[also build test ERROR on klassert-ipsec/master net/main net-next/main linus/master v6.10-rc3 next-20240612]
[cannot apply to horms-ipvs/master]
[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/Steffen-Klassert/xfrm-Fix-unregister-netdevice-hang-on-hardware-offload/20240612-171414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
patch link:    https://lore.kernel.org/r/ZmlmTTYL6AkBel4P%40gauss3.secunet.de
patch subject: [PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240612/202406122121.zejJ7IGG-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 4403cdbaf01379de96f8d0d6ea4f51a085e37766)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406122121.zejJ7IGG-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/202406122121.zejJ7IGG-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/xfrm/xfrm_state.c:19:
   In file included from include/net/xfrm.h:9:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/riscv/include/asm/cacheflush.h:9:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   net/xfrm/xfrm_state.c:688:6: error: redefinition of 'xfrm_dev_state_delete'
     688 | void xfrm_dev_state_delete(struct xfrm_state *x)
         |      ^
   include/net/xfrm.h:2022:20: note: previous definition is here
    2022 | static inline void xfrm_dev_state_delete(struct xfrm_state *x)
         |                    ^
>> net/xfrm/xfrm_state.c:694:8: error: no member named 'xfrmdev_ops' in 'struct net_device'
     694 |                 dev->xfrmdev_ops->xdo_dev_state_delete(x);
         |                 ~~~  ^
   net/xfrm/xfrm_state.c:701:6: error: redefinition of 'xfrm_dev_state_free'
     701 | void xfrm_dev_state_free(struct xfrm_state *x)
         |      ^
   include/net/xfrm.h:2026:20: note: previous definition is here
    2026 | static inline void xfrm_dev_state_free(struct xfrm_state *x)
         |                    ^
   net/xfrm/xfrm_state.c:706:18: error: no member named 'xfrmdev_ops' in 'struct net_device'; did you mean 'l3mdev_ops'?
     706 |         if (dev && dev->xfrmdev_ops) {
         |                         ^~~~~~~~~~~
         |                         l3mdev_ops
   include/linux/netdevice.h:2171:27: note: 'l3mdev_ops' declared here
    2171 |         const struct l3mdev_ops *l3mdev_ops;
         |                                  ^
   net/xfrm/xfrm_state.c:712:12: error: no member named 'xfrmdev_ops' in 'struct net_device'
     712 |                 if (dev->xfrmdev_ops->xdo_dev_state_free)
         |                     ~~~  ^
   net/xfrm/xfrm_state.c:713:9: error: no member named 'xfrmdev_ops' in 'struct net_device'
     713 |                         dev->xfrmdev_ops->xdo_dev_state_free(x);
         |                         ~~~  ^
   1 warning and 6 errors generated.


vim +694 net/xfrm/xfrm_state.c

   687	
   688	void xfrm_dev_state_delete(struct xfrm_state *x)
   689	{
   690		struct xfrm_dev_offload *xso = &x->xso;
   691		struct net_device *dev = READ_ONCE(xso->dev);
   692	
   693		if (dev) {
 > 694			dev->xfrmdev_ops->xdo_dev_state_delete(x);
   695			spin_lock_bh(&xfrm_state_dev_gc_lock);
   696			hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
   697			spin_unlock_bh(&xfrm_state_dev_gc_lock);
   698		}
   699	}
   700	

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

Re: [PATCH ipsec] xfrm: Fix unregister netdevice hang on hardware offload.

[Simon Horman]

On Wed, Jun 12, 2024 at 11:11:41AM +0200, Steffen Klassert wrote:
> When offloading xfrm states to hardware, the offloading
> device is attached to the skbs secpath. If a skb is free
> is deffered, an unregister netdevice hangs because the

Hi Steffen,

Some minor nits from my side as it looks like there will be a v2 anyway.

deffered -> deferred

Flagged by checkpatch.pl --codespell

> netdevice is still refcounted.
> 
> Fix this by removing the netdevice from the xfrm states
> when the netdevice is unregisterd. To find all xfrm states

nit: unregistered

> that need to be cleared we add another list where skbs
> linked to that are unlinked from the lists (deleted)
> but not yet freed.
> 
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> Tested-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

...