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

[Antony Antony <antony@phenome.org>]

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