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

[Sabrina Dubroca <sd@queasysnail.net>]

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