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

[Leon Romanovsky <leon@kernel.org>]

On Thu, Jun 20, 2024 at 08:47:24AM +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 deferred, an unregister netdevice hangs because the
> netdevice is still refcounted.
> 
> Fix this by removing the netdevice from the xfrm states
> when the netdevice is unregistered. 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.
> 
> Changes in v2:
> 
> - Fix build with CONFIG_XFRM_OFFLOAD disabled.
> - Fix two typos in the commit message.

Changelog should be after "---" trailer marker.

Thanks

> 
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  include/net/xfrm.h    | 36 +++++++------------------
>  net/xfrm/xfrm_state.c | 61 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 69 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..d531d2a1fae2 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,40 @@ struct xfrm_state *xfrm_state_alloc(struct net *net)
>  }
>  EXPORT_SYMBOL(xfrm_state_alloc);
>  
> +#ifdef CONFIG_XFRM_OFFLOAD
> +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);
> +	}
> +}
> +#endif
> +
>  void __xfrm_state_destroy(struct xfrm_state *x, bool sync)
>  {
>  	WARN_ON(x->km.state != XFRM_STATE_DEAD);
> @@ -848,6 +884,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 +896,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 +905,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 +923,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
> 
>