[PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error

[PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error

[Ren Wei]

From: Qianyu Luo <qianyuluo3@gmail.com>

nat_keepalive_send() frees the keepalive skb whenever the IPv4 or IPv6
send helper reports an error.

That cleanup is only correct before the skb is handed to the output
path. Once ip_build_and_send_pkt() or ip6_xmit() takes ownership, the
networking stack may already have consumed the skb before returning an
error, so freeing it again is unsafe.

Handle the pre-handoff failure cases inside nat_keepalive_send_ipv4()
and nat_keepalive_send_ipv6(), where the caller still owns the skb, and
keep nat_keepalive_send() responsible only for family dispatch and the
unsupported-family cleanup path.

Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
Cc: stable@vger.kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Qianyu Luo <qianyuluo3@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/xfrm/xfrm_nat_keepalive.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
index 458931062a04..f71328096f11 100644
--- a/net/xfrm/xfrm_nat_keepalive.c
+++ b/net/xfrm/xfrm_nat_keepalive.c
@@ -55,8 +55,10 @@ static int nat_keepalive_send_ipv4(struct sk_buff *skb,
 			   ka->encap_sport, sock_net_uid(net, NULL));
 
 	rt = ip_route_output_key(net, &fl4);
-	if (IS_ERR(rt))
+	if (IS_ERR(rt)) {
+		kfree_skb(skb);
 		return PTR_ERR(rt);
+	}
 
 	skb_dst_set(skb, &rt->dst);
 
@@ -100,6 +102,7 @@ static int nat_keepalive_send_ipv6(struct sk_buff *skb,
 	sock_net_set(sk, net);
 	dst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
 	if (IS_ERR(dst)) {
+		kfree_skb(skb);
 		local_unlock_nested_bh(&nat_keepalive_sk_ipv6.bh_lock);
 		return PTR_ERR(dst);
 	}
@@ -118,7 +121,6 @@ static void nat_keepalive_send(struct nat_keepalive *ka)
 					sizeof(struct ipv6hdr)) +
 				    sizeof(struct udphdr);
 	const u8 nat_ka_payload = 0xFF;
-	int err = -EAFNOSUPPORT;
 	struct sk_buff *skb;
 	struct udphdr *uh;
 
@@ -140,16 +142,17 @@ static void nat_keepalive_send(struct nat_keepalive *ka)
 
 	switch (ka->family) {
 	case AF_INET:
-		err = nat_keepalive_send_ipv4(skb, ka);
+		nat_keepalive_send_ipv4(skb, ka);
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		err = nat_keepalive_send_ipv6(skb, ka, uh);
+		nat_keepalive_send_ipv6(skb, ka, uh);
 		break;
 #endif
-	}
-	if (err)
+	default:
 		kfree_skb(skb);
+		break;
+	}
 }
 
 struct nat_keepalive_work_ctx {
-- 
2.43.7

Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error

[Eyal Birger]

On Thu, Jun 18, 2026 at 9:36 AM Ren Wei <n05ec@lzu.edu.cn> wrote:
>
> From: Qianyu Luo <qianyuluo3@gmail.com>
>
> nat_keepalive_send() frees the keepalive skb whenever the IPv4 or IPv6
> send helper reports an error.
>
> That cleanup is only correct before the skb is handed to the output
> path. Once ip_build_and_send_pkt() or ip6_xmit() takes ownership, the
> networking stack may already have consumed the skb before returning an
> error, so freeing it again is unsafe.
>
> Handle the pre-handoff failure cases inside nat_keepalive_send_ipv4()
> and nat_keepalive_send_ipv6(), where the caller still owns the skb, and
> keep nat_keepalive_send() responsible only for family dispatch and the
> unsupported-family cleanup path.

Thanks for the fix!

>
> Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
> Cc: stable@vger.kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Qianyu Luo <qianyuluo3@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>  net/xfrm/xfrm_nat_keepalive.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
> index 458931062a04..f71328096f11 100644
> --- a/net/xfrm/xfrm_nat_keepalive.c
> +++ b/net/xfrm/xfrm_nat_keepalive.c
> @@ -55,8 +55,10 @@ static int nat_keepalive_send_ipv4(struct sk_buff *skb,
>                            ka->encap_sport, sock_net_uid(net, NULL));
>
>         rt = ip_route_output_key(net, &fl4);
> -       if (IS_ERR(rt))
> +       if (IS_ERR(rt)) {
> +               kfree_skb(skb);
>                 return PTR_ERR(rt);
> +       }
>
>         skb_dst_set(skb, &rt->dst);
>
> @@ -100,6 +102,7 @@ static int nat_keepalive_send_ipv6(struct sk_buff *skb,
>         sock_net_set(sk, net);
>         dst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
>         if (IS_ERR(dst)) {
> +               kfree_skb(skb);
>                 local_unlock_nested_bh(&nat_keepalive_sk_ipv6.bh_lock);

Any reason to do the kfree under lock?

Eyal

Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error

[Qianyu Luo]

On Fri, Jun 19, 2026 at 1:21 AM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> On Thu, Jun 18, 2026 at 9:36 AM Ren Wei <n05ec@lzu.edu.cn> wrote:
> >
> > From: Qianyu Luo <qianyuluo3@gmail.com>
> >
> > nat_keepalive_send() frees the keepalive skb whenever the IPv4 or IPv6
> > send helper reports an error.
> >
> > That cleanup is only correct before the skb is handed to the output
> > path. Once ip_build_and_send_pkt() or ip6_xmit() takes ownership, the
> > networking stack may already have consumed the skb before returning an
> > error, so freeing it again is unsafe.
> >
> > Handle the pre-handoff failure cases inside nat_keepalive_send_ipv4()
> > and nat_keepalive_send_ipv6(), where the caller still owns the skb, and
> > keep nat_keepalive_send() responsible only for family dispatch and the
> > unsupported-family cleanup path.
>
> Thanks for the fix!
>
> >
> > Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
> > Cc: stable@vger.kernel.org
> > Reported-by: Yuan Tan <yuantan098@gmail.com>
> > Reported-by: Xin Liu <bird@lzu.edu.cn>
> > Signed-off-by: Qianyu Luo <qianyuluo3@gmail.com>
> > Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> > ---
> >  net/xfrm/xfrm_nat_keepalive.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
> > index 458931062a04..f71328096f11 100644
> > --- a/net/xfrm/xfrm_nat_keepalive.c
> > +++ b/net/xfrm/xfrm_nat_keepalive.c
> > @@ -55,8 +55,10 @@ static int nat_keepalive_send_ipv4(struct sk_buff *skb,
> >                            ka->encap_sport, sock_net_uid(net, NULL));
> >
> >         rt = ip_route_output_key(net, &fl4);
> > -       if (IS_ERR(rt))
> > +       if (IS_ERR(rt)) {
> > +               kfree_skb(skb);
> >                 return PTR_ERR(rt);
> > +       }
> >
> >         skb_dst_set(skb, &rt->dst);
> >
> > @@ -100,6 +102,7 @@ static int nat_keepalive_send_ipv6(struct sk_buff *skb,
> >         sock_net_set(sk, net);
> >         dst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
> >         if (IS_ERR(dst)) {
> > +               kfree_skb(skb);
> >                 local_unlock_nested_bh(&nat_keepalive_sk_ipv6.bh_lock);
>
> Any reason to do the kfree under lock?
>

Thank you for your reply! I did this without particular reason.
Just to keep the pre-handoff cleanup in the same error path.
kfree_skb() does not need the bh lock there, so I can move it after the
unlock in v2 if you need!

> Eyal

Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error

[Steffen Klassert]

On Fri, Jun 19, 2026 at 05:06:40PM +0800, Qianyu Luo wrote:
> On Fri, Jun 19, 2026 at 1:21 AM Eyal Birger <eyal.birger@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2026 at 9:36 AM Ren Wei <n05ec@lzu.edu.cn> wrote:
> > >
> > > From: Qianyu Luo <qianyuluo3@gmail.com>
> > >
> > >         skb_dst_set(skb, &rt->dst);
> > >
> > > @@ -100,6 +102,7 @@ static int nat_keepalive_send_ipv6(struct sk_buff *skb,
> > >         sock_net_set(sk, net);
> > >         dst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
> > >         if (IS_ERR(dst)) {
> > > +               kfree_skb(skb);
> > >                 local_unlock_nested_bh(&nat_keepalive_sk_ipv6.bh_lock);
> >
> > Any reason to do the kfree under lock?
> >
> 
> Thank you for your reply! I did this without particular reason.
> Just to keep the pre-handoff cleanup in the same error path.
> kfree_skb() does not need the bh lock there, so I can move it after the
> unlock in v2 if you need!

Then move it after the unlock to make it clear it is not needed.