* [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error [not found] <cover.1781788385.git.qianyuluo3@gmail.com> @ 2026-06-18 16:36 ` Ren Wei 2026-06-18 17:21 ` Eyal Birger 0 siblings, 1 reply; 4+ messages in thread From: Ren Wei @ 2026-06-18 16:36 UTC (permalink / raw) To: netdev Cc: steffen.klassert, herbert, davem, eyal.birger, yuantan098, bird, qianyuluo3, n05ec 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error 2026-06-18 16:36 ` [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error Ren Wei @ 2026-06-18 17:21 ` Eyal Birger 2026-06-19 9:06 ` Qianyu Luo 0 siblings, 1 reply; 4+ messages in thread From: Eyal Birger @ 2026-06-18 17:21 UTC (permalink / raw) To: Ren Wei Cc: netdev, steffen.klassert, herbert, davem, yuantan098, bird, qianyuluo3 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error 2026-06-18 17:21 ` Eyal Birger @ 2026-06-19 9:06 ` Qianyu Luo 2026-06-24 11:49 ` Steffen Klassert 0 siblings, 1 reply; 4+ messages in thread From: Qianyu Luo @ 2026-06-19 9:06 UTC (permalink / raw) To: Eyal Birger Cc: Ren Wei, netdev, steffen.klassert, herbert, davem, yuantan098, bird 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error 2026-06-19 9:06 ` Qianyu Luo @ 2026-06-24 11:49 ` Steffen Klassert 0 siblings, 0 replies; 4+ messages in thread From: Steffen Klassert @ 2026-06-24 11:49 UTC (permalink / raw) To: Qianyu Luo; +Cc: Eyal Birger, Ren Wei, netdev, herbert, davem, yuantan098, bird 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-06-24 11:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1781788385.git.qianyuluo3@gmail.com>
2026-06-18 16:36 ` [PATCH 1/1] xfrm: nat_keepalive: avoid double free on send error Ren Wei
2026-06-18 17:21 ` Eyal Birger
2026-06-19 9:06 ` Qianyu Luo
2026-06-24 11:49 ` Steffen Klassert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox