netdev - Re: [devel-ipsec] Re: [PATCH ipsec-next 4/6] xfrm: add XFRM_MSG_MIGRATE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aWogXn5hUay3iTd_@horms.kernel.org>
Date: Fri, 16 Jan 2026 11:26:22 +0000
From: Simon Horman <horms@...nel.org>
To: Antony Antony <antony@...nome.org>
Cc: Antony Antony <antony.antony@...unet.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	devel@...ux-ipsec.org
Subject: Re: [devel-ipsec] Re: [PATCH ipsec-next 4/6] xfrm: add
 XFRM_MSG_MIGRATE_STATE for single SA migration

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:
> > On Wed, Jan 14, 2026 at 05:09:20PM +0100, Antony Antony wrote:
> > 
> > Hi Antony,
> > 
> > > Hi Simon,
> > > 
> > > On Tue, Jan 13, 2026 at 02:57:16PM +0000, Simon Horman via Devel wrote:
> > > > On Fri, Jan 09, 2026 at 02:38:05PM +0100, Antony Antony wrote:
> > 
> > ...
> > 
> > > > > +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 bellow
> 
> > > > > +		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!