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

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

[Steffen Klassert]

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.

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

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

[Sabrina Dubroca]

2026-05-06, 18:08:55 +0200, Steffen Klassert 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.

Nice clean up!

[...]
> @@ -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;
> +	}
> +

nit: extra blank line

>  
>  	return nlmsg_unicast(xfrm_net_nlsk(net, skb), r_skb, sportid);
>  }

[...]
> @@ -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;

This one is missing the kfree_skb(skb).

>  
>  	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
>  }

-- 
Sabrina

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

[Antony Antony]

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
> 

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

[Sabrina Dubroca]

2026-05-07, 06:21:57 +0100, Antony Antony wrote:
> 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;
> +     }

OTOH we already have a bunch of functions doing something similar
without using BUG_ON/WARN_ON, so at least with this patch it becomes
consistent.

xfrm_notify_userpolicy
xfrm_get_default
xfrm_get_ae
xfrm_exp_state_notify
xfrm_notify_sa_flush
xfrm_notify_sa
xfrm_notify_policy
xfrm_notify_policy_flush


(I'm looking into generic ways to avoid this split getsize/fill that
always becomes inconsistent in areas where new attributes are added
frequently, but nothing to share yet)

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

Maybe DEBUG_NET_WARN_ON_ONCE so that only developers see those messages.

-- 
Sabrina

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

[Antony Antony]

On Thu, May 07, 2026 at 10:11:36AM +0200, Sabrina Dubroca via Devel wrote:
> 2026-05-07, 06:21:57 +0100, Antony Antony wrote:
> > 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;
> > +     }
> 
> OTOH we already have a bunch of functions doing something similar
> without using BUG_ON/WARN_ON, so at least with this patch it becomes
> consistent.
> 
> xfrm_notify_userpolicy
> xfrm_get_default
> xfrm_get_ae
> xfrm_exp_state_notify
> xfrm_notify_sa_flush
> xfrm_notify_sa
> xfrm_notify_policy
> xfrm_notify_policy_flush
> 
> 
> (I'm looking into generic ways to avoid this split getsize/fill that
> always becomes inconsistent in areas where new attributes are added
> frequently, but nothing to share yet)
> 
> > 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?
> 
> Maybe DEBUG_NET_WARN_ON_ONCE so that only developers see those messages.


I would discourage DEBUG_NET_* here, because these situations are not often
caught by testing or by developers. Typically IPsec builds disable debug 
completely - and I would even speculate that is why it was BUG_ON in the 
early days. If IPsec failed, halting was justified back then. While these 
days it is less so.

I vote for something along the lines of WARN_*. That has a higher chance of gathering better diagnostics for developers.

Would other IKE userspace developers like to chime in?

-antony