Re: [PATCH ipsec-next] xfrm: Use regular error handling instead of BUG_ON() in the netlink API.

[Antony Antony <antony@phenome.org>]

wHi Steffen,

Thanks Steffen, I was hit by this in the new XFRM_MIGRATE_STATE I am adding.
I am glad to see we are addressing this.

On Wed, May 06, 2026 at 06:08:55PM +0200, Steffen Klassert via Devel wrote:
> The xfrm netlink API uses BUG_ON() on failures since it exists.
> However all these error are uncritical and can be handled
> with regular error handling. This fixes machine crashes
> in situations where an emergency break is not needed.

While BUG_ON is an extreme measure for a recoverable netlink error, it does
have diagnostic value: it leaves a stack trace. The patch trades
a crash + stack trace for a silent error return, which loses observability.

Would you consider using WARN_ONCE instead of a bare if (err < 0)?

-     BUG_ON(err < 0);
+     if (WARN_ONCE(err < 0, "xfrm: build_spdinfo failed: %d\n", err)) {
+         kfree_skb(r_skb);
+         return err;
+     }


Something like the above would preserve the "shouldn't happen" signal with a 
stack trace on first occurrence, without panicking the machine.
Or are there better signaling  styles in Kernel?

> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/xfrm/xfrm_user.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index d56450f61669..b24a0f9e91d5 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1734,7 +1734,10 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -ENOMEM;
>  
>  	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(r_skb);
> +		return err;
> +	}
>  
>  	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, sportid);
>  }
> @@ -1794,7 +1797,11 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -ENOMEM;
>  
>  	err = build_sadinfo(r_skb, net, sportid, seq, *flags);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(r_skb);
> +		return err;
> +	}
> +
>  
>  	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, sportid);
>  }
> @@ -3285,7 +3292,10 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
>  
>  	/* build migrate */
>  	err = build_migrate(skb, m, num_migrate, k, sel, encap, dir, type);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(skb);
> +		return err;
> +	}
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
>  }
> @@ -3623,7 +3633,10 @@ static int xfrm_aevent_state_notify(struct xfrm_state *x, const struct km_event
>  		return -ENOMEM;
>  
>  	err = build_aevent(skb, x, c);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(skb);
> +		return err;
> +	}
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_AEVENTS);
>  }
> @@ -3862,7 +3875,10 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
>  		return -ENOMEM;
>  
>  	err = build_acquire(skb, x, xt, xp);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(skb);
> +		return err;
> +	}
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_ACQUIRE);
>  }
> @@ -3984,7 +4000,10 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, const struct
>  		return -ENOMEM;
>  
>  	err = build_polexpire(skb, xp, dir, c);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(skb);
> +		return err;
> +	}
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_EXPIRE);
>  }
> @@ -4151,7 +4170,8 @@ static int xfrm_send_report(struct net *net, u8 proto,
>  		return -ENOMEM;
>  
>  	err = build_report(skb, proto, sel, addr);
> -	BUG_ON(err < 0);
> +	if (err < 0)
> +		return err;
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
>  }
> @@ -4206,7 +4226,10 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
>  		return -ENOMEM;
>  
>  	err = build_mapping(skb, x, ipaddr, sport);
> -	BUG_ON(err < 0);
> +	if (err < 0) {
> +		kfree_skb(skb);
> +		return err;
> +	}
>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MAPPING);
>  }
> -- 
> 2.43.0
>