Re: [devel-ipsec] Re: [PATCH ipsec-next 4/6] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration
From: Simon Horman <horms@kernel.org>
Date: 2026-01-16 11:26:27
On Fri, Jan 16, 2026 at 12:02:12PM +0100, Antony Antony wrote:
On Thu, Jan 15, 2026 at 01:44:50PM +0000, Simon Horman via Devel wrote:quoted
On Wed, Jan 14, 2026 at 05:09:20PM +0100, Antony Antony wrote: Hi Antony,quoted
Hi Simon, On Tue, Jan 13, 2026 at 02:57:16PM +0000, Simon Horman via Devel wrote:quoted
On Fri, Jan 09, 2026 at 02:38:05PM +0100, Antony Antony wrote:...quoted
quoted
quoted
+static int xfrm_send_migrate_state(const struct xfrm_user_migrate_state *um, + const struct xfrm_encap_tmpl *encap, + const struct xfrm_user_offload *xuo) +{ + int err; + struct sk_buff *skb; + struct net *net = &init_net; + + skb = nlmsg_new(xfrm_migrate_state_msgsize(!!encap, !!xuo), GFP_ATOMIC); + if (!skb) + return -ENOMEM; + + err = build_migrate_state(skb, um, encap, xuo); + if (err < 0) { + WARN_ON(1);kfree_skb(skb); replace the above line; explained bellowquoted
quoted
quoted
quoted
+ return err;skb seems to be leaked here. Also flagged by Review Prompts.I don't see a skb leak. It also looks similar to the functions above.xfrm_get_ae() is the previous caller of nlmsg_new() in this file. It calls BUG_ON() on error, so leaking is not an issue there. The caller before that is xfrm_get_default() which calls kfree_skb() in it's error path. Maybe I'm missing something obvious, but I was thinking that approach is appropriate here too.You’re right. There is a leak in the error path. The new helper I added is similar to build_migrate(), but that code uses BUG_ON() on the error path. That feels too extreme here (even though there are other instances of it in the same file).
FWIIW, the use of BUG_ON in xfrm_get_ae() did give me pause for thought.
I’ll follow the pattern in xfrm_get_default(): handle the error by freeing the skb (kfree_skb()) and returning an error. And no WARN_ON(). I’ll send v3 shortly.
Thanks!