When the skb is reorganized during esp_output (!esp->inline), the pages coming from the original skb fragments are supposed to be released back to the system through put_page. But if the skb fragment pages are originating from a page_pool, calling put_page on them will trigger a page_pool leak which will eventually result in a crash.
This leak can be easily observed when using CONFIG_DEBUG_VM and doing ipsec + gre (non offloaded) forwarding:
BUG: Bad page state in process ksoftirqd/16 pfn:1451b6 page:00000000de2b8d32 refcount:0 mapcount:0 mapping:0000000000000000 index:0x1451b6000 pfn:0x1451b6 flags: 0x200000000000000(node=0|zone=2) page_type: 0xffffffff() raw: 0200000000000000 dead000000000040 ffff88810d23c000 0000000000000000 raw: 00000001451b6000 0000000000000001 00000000ffffffff 0000000000000000 page dumped because: page_pool leak Modules linked in: ip_gre gre mlx5_ib mlx5_core xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink iptable_nat nf_nat xt_addrtype br_netfilter rpcrdma rdma_ucm ib_iser libiscsi scsi_transport_iscsi ib_umad rdma_cm ib_ipoib iw_cm ib_cm ib_uverbs ib_core overlay zram zsmalloc fuse [last unloaded: mlx5_core] CPU: 16 PID: 96 Comm: ksoftirqd/16 Not tainted 6.8.0-rc4+ #22 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x36/0x50 bad_page+0x70/0xf0 free_unref_page_prepare+0x27a/0x460 free_unref_page+0x38/0x120 esp_ssg_unref.isra.0+0x15f/0x200 esp_output_tail+0x66d/0x780 esp_xmit+0x2c5/0x360 validate_xmit_xfrm+0x313/0x370 ? validate_xmit_skb+0x1d/0x330 validate_xmit_skb_list+0x4c/0x70 sch_direct_xmit+0x23e/0x350 __dev_queue_xmit+0x337/0xba0 ? nf_hook_slow+0x3f/0xd0 ip_finish_output2+0x25e/0x580 iptunnel_xmit+0x19b/0x240 ip_tunnel_xmit+0x5fb/0xb60 ipgre_xmit+0x14d/0x280 [ip_gre] dev_hard_start_xmit+0xc3/0x1c0 __dev_queue_xmit+0x208/0xba0 ? nf_hook_slow+0x3f/0xd0 ip_finish_output2+0x1ca/0x580 ip_sublist_rcv_finish+0x32/0x40 ip_sublist_rcv+0x1b2/0x1f0 ? ip_rcv_finish_core.constprop.0+0x460/0x460 ip_list_rcv+0x103/0x130 __netif_receive_skb_list_core+0x181/0x1e0 netif_receive_skb_list_internal+0x1b3/0x2c0 napi_gro_receive+0xc8/0x200 gro_cell_poll+0x52/0x90 __napi_poll+0x25/0x1a0 net_rx_action+0x28e/0x300 __do_softirq+0xc3/0x276 ? sort_range+0x20/0x20 run_ksoftirqd+0x1e/0x30 smpboot_thread_fn+0xa6/0x130 kthread+0xcd/0x100 ? kthread_complete_and_exit+0x20/0x20 ret_from_fork+0x31/0x50 ? kthread_complete_and_exit+0x20/0x20 ret_from_fork_asm+0x11/0x20 </TASK>
The suggested fix is to introduce a new wrapper (skb_page_unref) that covers page refcounting for page_pool pages as well.
Signed-off-by: Dragos Tatulea dtatulea@nvidia.com Reported-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz Tested-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz Reported-by: Ian Kumlien ian.kumlien@gmail.com Closes: https: //lore.kernel.org/netdev/CAA85sZvvHtrpTQRqdaOx6gd55zPAVsqMYk_Lwh4Md5knTq7AyA@mail.gmail.com Cc: stable@vger.kernel.org Reviewed-by: Mina Almasry almasrymina@google.com Change-Id: I3d2744e1abb33a694a8f49e07f913724a0f8871a --- include/linux/skbuff.h | 10 ++++++++++ net/ipv4/esp4.c | 8 ++++---- net/ipv6/esp6.c | 8 ++++---- 3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 696e7680656f..6126fc8e4a89 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3452,6 +3452,16 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb, struct bpf_prog *prog); bool napi_pp_put_page(struct page *page, bool napi_safe);
+static inline void +skb_page_unref(const struct sk_buff *skb, struct page *page, bool napi_safe) +{ +#ifdef CONFIG_PAGE_POOL + if (skb->pp_recycle && napi_pp_put_page(page, napi_safe)) + return; +#endif + put_page(page); +} + static inline void napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe) { diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c index 4dd9e5040672..d33d12421814 100644 --- a/net/ipv4/esp4.c +++ b/net/ipv4/esp4.c @@ -95,7 +95,7 @@ static inline struct scatterlist *esp_req_sg(struct crypto_aead *aead, __alignof__(struct scatterlist)); }
-static void esp_ssg_unref(struct xfrm_state *x, void *tmp) +static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb) { struct crypto_aead *aead = x->data; int extralen = 0; @@ -114,7 +114,7 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp) */ if (req->src != req->dst) for (sg = sg_next(req->src); sg; sg = sg_next(sg)) - put_page(sg_page(sg)); + skb_page_unref(skb, sg_page(sg), false); }
#ifdef CONFIG_INET_ESPINTCP @@ -260,7 +260,7 @@ static void esp_output_done(void *data, int err) }
tmp = ESP_SKB_CB(skb)->tmp; - esp_ssg_unref(x, tmp); + esp_ssg_unref(x, tmp, skb); kfree(tmp);
if (xo && (xo->flags & XFRM_DEV_RESUME)) { @@ -639,7 +639,7 @@ int esp_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info * }
if (sg != dsg) - esp_ssg_unref(x, tmp); + esp_ssg_unref(x, tmp, skb);
if (!err && x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP) err = esp_output_tail_tcp(x, skb); diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c index 6e6efe026cdc..7371886d4f9f 100644 --- a/net/ipv6/esp6.c +++ b/net/ipv6/esp6.c @@ -112,7 +112,7 @@ static inline struct scatterlist *esp_req_sg(struct crypto_aead *aead, __alignof__(struct scatterlist)); }
-static void esp_ssg_unref(struct xfrm_state *x, void *tmp) +static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb) { struct crypto_aead *aead = x->data; int extralen = 0; @@ -131,7 +131,7 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp) */ if (req->src != req->dst) for (sg = sg_next(req->src); sg; sg = sg_next(sg)) - put_page(sg_page(sg)); + skb_page_unref(skb, sg_page(sg), false); }
#ifdef CONFIG_INET6_ESPINTCP @@ -294,7 +294,7 @@ static void esp_output_done(void *data, int err) }
tmp = ESP_SKB_CB(skb)->tmp; - esp_ssg_unref(x, tmp); + esp_ssg_unref(x, tmp, skb); kfree(tmp);
esp_output_encap_csum(skb); @@ -677,7 +677,7 @@ int esp6_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info }
if (sg != dsg) - esp_ssg_unref(x, tmp); + esp_ssg_unref(x, tmp, skb);
if (!err && x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP) err = esp_output_tail_tcp(x, skb);
LGTM, thanks! Minor nit picks for the tags, otherise:
Reviewed-by: Jakub Kicinski kuba@kernel.org
On Thu, 7 Mar 2024 12:15:47 +0200 Dragos Tatulea wrote:
Signed-off-by: Dragos Tatulea dtatulea@nvidia.com
General guidance is to order tag chronologically, so:
Cc: stable@vger.kernel.org Fixes: (speaking of which, you need to add it, presumably point it at the commit where skb->pp_recycle was added) Reported-by: Link / Closes: your sign-off Review tags
Reported-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz Tested-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz
You can combine it as Reported-and-tested-by: up to you
Reported-by: Ian Kumlien ian.kumlien@gmail.com Closes: https: //lore.kernel.org/netdev/CAA85sZvvHtrpTQRqdaOx6gd55zPAVsqMYk_Lwh4Md5knTq7AyA@mail.gmail.com
A space sneaked into the link.
Cc: stable@vger.kernel.org Reviewed-by: Mina Almasry almasrymina@google.com Change-Id: I3d2744e1abb33a694a8f49e07f913724a0f8871a
Change-Id's gotta go.
On Thu, 2024-03-07 at 09:51 -0800, Jakub Kicinski wrote:
LGTM, thanks! Minor nit picks for the tags, otherise:
Reviewed-by: Jakub Kicinski kuba@kernel.org
Thanks!
On Thu, 7 Mar 2024 12:15:47 +0200 Dragos Tatulea wrote:
Signed-off-by: Dragos Tatulea dtatulea@nvidia.com
General guidance is to order tag chronologically, so:
Cc: stable@vger.kernel.org Fixes: (speaking of which, you need to add it, presumably point it at the commit where skb->pp_recycle was added) Reported-by: Link / Closes: your sign-off Review tags
Reported-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz Tested-by: Anatoli N.Chechelnickiy Anatoli.Chechelnickiy@m.interpipe.biz
You can combine it as Reported-and-tested-by: up to you
Reported-by: Ian Kumlien ian.kumlien@gmail.com Closes: https: //lore.kernel.org/netdev/CAA85sZvvHtrpTQRqdaOx6gd55zPAVsqMYk_Lwh4Md5knTq7AyA@mail.gmail.com
A space sneaked into the link.
Cc: stable@vger.kernel.org Reviewed-by: Mina Almasry almasrymina@google.com Change-Id: I3d2744e1abb33a694a8f49e07f913724a0f8871a
Change-Id's gotta go.
Fixed tags in v2. Thanks for explaining the tag order. It was never really clear to me.
Thanks, Dragos
linux-stable-mirror@lists.linaro.org
-
Dragos Tatulea -
Jakub Kicinski