[PATCH] xfrm: fix memory leak in xfrm_add_acquire()

[PATCH] xfrm: fix memory leak in xfrm_add_acquire()

[Zilin Guan]

xfrm_add_acquire() constructs an xfrm_policy by calling
xfrm_policy_construct(), which allocates the policy structure via
xfrm_policy_alloc() and initializes its security context.

However, xfrm_add_acquire() currently releases the policy with kfree(),
which skips the proper cleanup and causes a memory leak.

Fix this by calling xfrm_policy_destroy() instead of kfree() to
properly release the policy and its associated resources, consistent
with the cleanup path in xfrm_policy_construct().

Fixes: 980ebd25794f ("[IPSEC]: Sync series - acquire insert")
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 net/xfrm/xfrm_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 010c9e6638c0..23c9bb42bb2a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3035,7 +3035,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	xfrm_state_free(x);
-	kfree(xp);
+	xfrm_policy_destroy(xp);
 
 	return 0;
 
-- 
2.34.1

Re: [PATCH] xfrm: fix memory leak in xfrm_add_acquire()

[Sabrina Dubroca]

2025-11-08, 05:10:54 +0000, Zilin Guan wrote:
> xfrm_add_acquire() constructs an xfrm_policy by calling
> xfrm_policy_construct(), which allocates the policy structure via
> xfrm_policy_alloc() and initializes its security context.
> 
> However, xfrm_add_acquire() currently releases the policy with kfree(),
> which skips the proper cleanup and causes a memory leak.
> 
> Fix this by calling xfrm_policy_destroy() instead of kfree() to
> properly release the policy and its associated resources, consistent
> with the cleanup path in xfrm_policy_construct().
> 
> Fixes: 980ebd25794f ("[IPSEC]: Sync series - acquire insert")
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  net/xfrm/xfrm_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 010c9e6638c0..23c9bb42bb2a 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -3035,7 +3035,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	}
>  
>  	xfrm_state_free(x);
> -	kfree(xp);
> +	xfrm_policy_destroy(xp);

I agree there's something missing here, but that's not the right way
to fix this. You're calling this function:

void xfrm_policy_destroy(struct xfrm_policy *policy)
{
	BUG_ON(!policy->walk.dead);
[...]


And xfrm_add_acquire is not setting walk.dead. Have you tested your
patch?

Even if we did set walk.dead before calling xfrm_policy_destroy, we
would still be missing the xfrm_dev_policy_delete call that is done in
xfrm_policy_kill for the normal policy cleanup path.

I think we want something more like what xfrm_add_policy does if
insertion fails. In xfrm_policy_construct (which you mention in the
commit message), we don't have to worry about xfrm_dev_policy_delete
because xfrm_dev_policy_add has either not been called at all, or has
failed and does not need extra cleanup.

-- 
Sabrina

Re: [PATCH] xfrm: fix memory leak in xfrm_add_acquire()

[Zilin Guan]

On Sat, Nov 08, 2025 at 11:08:15AM +0100, Sabrina Dubroca wrote:
> 2025-11-08, 05:10:54 +0000, Zilin Guan wrote:
> > xfrm_add_acquire() constructs an xfrm_policy by calling
> > xfrm_policy_construct(), which allocates the policy structure via
> > xfrm_policy_alloc() and initializes its security context.
> > 
> > However, xfrm_add_acquire() currently releases the policy with kfree(),
> > which skips the proper cleanup and causes a memory leak.
> > 
> > Fix this by calling xfrm_policy_destroy() instead of kfree() to
> > properly release the policy and its associated resources, consistent
> > with the cleanup path in xfrm_policy_construct().
> > 
> > Fixes: 980ebd25794f ("[IPSEC]: Sync series - acquire insert")
> > Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> > ---
> >  net/xfrm/xfrm_user.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index 010c9e6638c0..23c9bb42bb2a 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -3035,7 +3035,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  	}
> >  
> >  	xfrm_state_free(x);
> > -	kfree(xp);
> > +	xfrm_policy_destroy(xp);
> 
> I agree there's something missing here, but that's not the right way
> to fix this. You're calling this function:
> 
> void xfrm_policy_destroy(struct xfrm_policy *policy)
> {
> 	BUG_ON(!policy->walk.dead);
> [...]
> 
> 
> And xfrm_add_acquire is not setting walk.dead. Have you tested your
> patch?

My apologies, I see the mistake now. To answer your question, I found 
this issue through static analysis and failed to test the patch properly 
before submission.

> Even if we did set walk.dead before calling xfrm_policy_destroy, we
> would still be missing the xfrm_dev_policy_delete call that is done in
> xfrm_policy_kill for the normal policy cleanup path.

Thank you for pointing this out. I agree that the xfrm_dev_policy_delete() 
call is also necessary.

> I think we want something more like what xfrm_add_policy does if
> insertion fails. In xfrm_policy_construct (which you mention in the
> commit message), we don't have to worry about xfrm_dev_policy_delete
> because xfrm_dev_policy_add has either not been called at all, or has
> failed and does not need extra cleanup.
> 
> -- 
> Sabrina

Thank you for the detailed review and suggestion. I will follow the 
error handling pattern in xfrm_add_policy() and prepare a v2 patch 
accordingly.

Best regards,
Zilin Guan