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 use a new wrapper that covers page refcounting
for page_pool pages as well.
Signed-off-by: Dragos Tatulea <[email protected]>
Reported-by: Anatoli N.Chechelnickiy <[email protected]>
Reported-by: Ian Kumlien <[email protected]>
---
Changes in v2:
- Added napi_page_unref api based on discussion in v1 [0].
[0]https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t
---
include/linux/skbuff.h | 10 +++++++---
net/ipv4/esp4.c | 16 ++++++++++------
net/ipv6/esp6.c | 16 ++++++++++------
3 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 696e7680656f..009603db2a43 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3453,10 +3453,8 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
bool napi_pp_put_page(struct page *page, bool napi_safe);
static inline void
-napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
+napi_page_unref(struct page *page, bool recycle, bool napi_safe)
{
- struct page *page = skb_frag_page(frag);
-
#ifdef CONFIG_PAGE_POOL
if (recycle && napi_pp_put_page(page, napi_safe))
return;
@@ -3464,6 +3462,12 @@ napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
put_page(page);
}
+static inline void
+napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
+{
+ napi_page_unref(skb_frag_page(frag), recycle, napi_safe);
+}
+
/**
* __skb_frag_unref - release a reference on a paged fragment.
* @frag: the paged fragment
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 4dd9e5040672..3126aeb0588d 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;
@@ -112,9 +112,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
/* Unref skb_frag_pages in the src scatterlist if necessary.
* Skip the first sg which comes from skb->data.
*/
- if (req->src != req->dst)
- for (sg = sg_next(req->src); sg; sg = sg_next(sg))
- put_page(sg_page(sg));
+ if (req->src != req->dst) {
+ for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
+ struct page *page = sg_page(sg);
+
+ napi_page_unref(page, skb->pp_recycle, false);
+ }
+ }
}
#ifdef CONFIG_INET_ESPINTCP
@@ -260,7 +264,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 +643,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..7bbe19e74cf0 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;
@@ -129,9 +129,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
/* Unref skb_frag_pages in the src scatterlist if necessary.
* Skip the first sg which comes from skb->data.
*/
- if (req->src != req->dst)
- for (sg = sg_next(req->src); sg; sg = sg_next(sg))
- put_page(sg_page(sg));
+ if (req->src != req->dst) {
+ for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
+ struct page *page = sg_page(sg);
+
+ napi_page_unref(page, skb->pp_recycle, false);
+ }
+ }
}
#ifdef CONFIG_INET6_ESPINTCP
@@ -294,7 +298,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 +681,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);
--
2.42.0
On Wed, 2024-03-06 at 21:08 +0200, Dragos Tatulea wrote:
> 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 use a new wrapper that covers page refcounting
> for page_pool pages as well.
>
> Signed-off-by: Dragos Tatulea <[email protected]>
> Reported-by: Anatoli N.Chechelnickiy <[email protected]>
> Reported-by: Ian Kumlien <[email protected]>
> ---
>
> Changes in v2:
> - Added napi_page_unref api based on discussion in v1 [0].
>
> [0]https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t
>
Jakub, are you sure that it is ok to have all this in a single patch?
Thanks,
Dragos
> ---
> include/linux/skbuff.h | 10 +++++++---
> net/ipv4/esp4.c | 16 ++++++++++------
> net/ipv6/esp6.c | 16 ++++++++++------
> 3 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 696e7680656f..009603db2a43 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3453,10 +3453,8 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
> bool napi_pp_put_page(struct page *page, bool napi_safe);
>
> static inline void
> -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> +napi_page_unref(struct page *page, bool recycle, bool napi_safe)
> {
> - struct page *page = skb_frag_page(frag);
> -
> #ifdef CONFIG_PAGE_POOL
> if (recycle && napi_pp_put_page(page, napi_safe))
> return;
> @@ -3464,6 +3462,12 @@ napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> put_page(page);
> }
>
> +static inline void
> +napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> +{
> + napi_page_unref(skb_frag_page(frag), recycle, napi_safe);
> +}
> +
> /**
> * __skb_frag_unref - release a reference on a paged fragment.
> * @frag: the paged fragment
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index 4dd9e5040672..3126aeb0588d 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;
> @@ -112,9 +112,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> /* Unref skb_frag_pages in the src scatterlist if necessary.
> * Skip the first sg which comes from skb->data.
> */
> - if (req->src != req->dst)
> - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> - put_page(sg_page(sg));
> + if (req->src != req->dst) {
> + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> + struct page *page = sg_page(sg);
> +
> + napi_page_unref(page, skb->pp_recycle, false);
> + }
> + }
> }
>
> #ifdef CONFIG_INET_ESPINTCP
> @@ -260,7 +264,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 +643,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..7bbe19e74cf0 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;
> @@ -129,9 +129,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> /* Unref skb_frag_pages in the src scatterlist if necessary.
> * Skip the first sg which comes from skb->data.
> */
> - if (req->src != req->dst)
> - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> - put_page(sg_page(sg));
> + if (req->src != req->dst) {
> + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> + struct page *page = sg_page(sg);
> +
> + napi_page_unref(page, skb->pp_recycle, false);
> + }
> + }
> }
>
> #ifdef CONFIG_INET6_ESPINTCP
> @@ -294,7 +298,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 +681,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);
On Wed, Mar 6, 2024 at 11:08 AM Dragos Tatulea <[email protected]> wrote:
>
> 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 use a new wrapper that covers page refcounting
> for page_pool pages as well.
>
> Signed-off-by: Dragos Tatulea <[email protected]>
> Reported-by: Anatoli N.Chechelnickiy <[email protected]>
> Reported-by: Ian Kumlien <[email protected]>
Maybe you want to Cc: stable on this & add fixes tag? Or maybe you
skipped that because it's still an RFC patch.
I think it maybe also needs to be [PATCH net] or [RFC PATCH net]
subject to designate to net and not net-next.
Looks good to me with very minor comment so, FWIW,
Reviewed-by: Mina Almasry <[email protected]>
> ---
>
> Changes in v2:
> - Added napi_page_unref api based on discussion in v1 [0].
>
> [0]https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t
>
> ---
> include/linux/skbuff.h | 10 +++++++---
> net/ipv4/esp4.c | 16 ++++++++++------
> net/ipv6/esp6.c | 16 ++++++++++------
> 3 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 696e7680656f..009603db2a43 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3453,10 +3453,8 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
> bool napi_pp_put_page(struct page *page, bool napi_safe);
>
> static inline void
> -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> +napi_page_unref(struct page *page, bool recycle, bool napi_safe)
> {
> - struct page *page = skb_frag_page(frag);
> -
> #ifdef CONFIG_PAGE_POOL
> if (recycle && napi_pp_put_page(page, napi_safe))
Ah, it looks like we don't need is_pp_page() after all because
napi_pp_put_page() already checks for that internally. Good find, much
better than what I suggested.
> return;
> @@ -3464,6 +3462,12 @@ napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> put_page(page);
> }
>
> +static inline void
> +napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> +{
> + napi_page_unref(skb_frag_page(frag), recycle, napi_safe);
> +}
> +
> /**
> * __skb_frag_unref - release a reference on a paged fragment.
> * @frag: the paged fragment
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index 4dd9e5040672..3126aeb0588d 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;
> @@ -112,9 +112,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> /* Unref skb_frag_pages in the src scatterlist if necessary.
> * Skip the first sg which comes from skb->data.
> */
> - if (req->src != req->dst)
> - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> - put_page(sg_page(sg));
> + if (req->src != req->dst) {
> + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> + struct page *page = sg_page(sg);
> +
> + napi_page_unref(page, skb->pp_recycle, false);
[minor annoying nit, feel free to ignore]: I would use sg_page(sg)
inline here and below, since page is not reused elsewhere. But I'm
fine either way.
> + }
> + }
> }
>
> #ifdef CONFIG_INET_ESPINTCP
> @@ -260,7 +264,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 +643,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..7bbe19e74cf0 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;
> @@ -129,9 +129,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> /* Unref skb_frag_pages in the src scatterlist if necessary.
> * Skip the first sg which comes from skb->data.
> */
> - if (req->src != req->dst)
> - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> - put_page(sg_page(sg));
> + if (req->src != req->dst) {
> + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> + struct page *page = sg_page(sg);
> +
> + napi_page_unref(page, skb->pp_recycle, false);
> + }
> + }
> }
>
> #ifdef CONFIG_INET6_ESPINTCP
> @@ -294,7 +298,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 +681,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);
> --
> 2.42.0
>
--
Thanks,
Mina
On Wed, 6 Mar 2024 19:09:58 +0000 Dragos Tatulea wrote:
> > Changes in v2:
> > - Added napi_page_unref api based on discussion in v1 [0].
> >
> > [0]https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t
> >
> Jakub, are you sure that it is ok to have all this in a single patch?
Yup.
> > ---
> > include/linux/skbuff.h | 10 +++++++---
> > net/ipv4/esp4.c | 16 ++++++++++------
> > net/ipv6/esp6.c | 16 ++++++++++------
> > 3 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 696e7680656f..009603db2a43 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3453,10 +3453,8 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
> > bool napi_pp_put_page(struct page *page, bool napi_safe);
> >
> > static inline void
> > -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > +napi_page_unref(struct page *page, bool recycle, bool napi_safe)
I'd call it skb_page_unref()
The napi_ prefix will just confuse people, because we don't have to be
in NAPI context at all when calling this. As long as "napi_safe" is
correctly set to false. So let's use skb_ as the prefix.
And I'd pass skb to this, not "bool recycle" so that the caller doesn't
have to know the weird details..
On Wed, 2024-03-06 at 13:15 -0800, Mina Almasry wrote:
> On Wed, Mar 6, 2024 at 11:08 AM Dragos Tatulea <[email protected]> wrote:
> >
> > 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 use a new wrapper that covers page refcounting
> > for page_pool pages as well.
> >
> > Signed-off-by: Dragos Tatulea <[email protected]>
> > Reported-by: Anatoli N.Chechelnickiy <[email protected]>
> > Reported-by: Ian Kumlien <[email protected]>
>
> Maybe you want to Cc: stable on this & add fixes tag? Or maybe you
> skipped that because it's still an RFC patch.
>
> I think it maybe also needs to be [PATCH net] or [RFC PATCH net]
> subject to designate to net and not net-next.
>
Will send the non RFC version with the forms filled in correctly :).
> Looks good to me with very minor comment so, FWIW,
>
> Reviewed-by: Mina Almasry <[email protected]>
>
Thanks!
> > ---
> >
> > Changes in v2:
> > - Added napi_page_unref api based on discussion in v1 [0].
> >
> > [0]https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t
> >
> > ---
> > include/linux/skbuff.h | 10 +++++++---
> > net/ipv4/esp4.c | 16 ++++++++++------
> > net/ipv6/esp6.c | 16 ++++++++++------
> > 3 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 696e7680656f..009603db2a43 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3453,10 +3453,8 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
> > bool napi_pp_put_page(struct page *page, bool napi_safe);
> >
> > static inline void
> > -napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > +napi_page_unref(struct page *page, bool recycle, bool napi_safe)
> > {
> > - struct page *page = skb_frag_page(frag);
> > -
> > #ifdef CONFIG_PAGE_POOL
> > if (recycle && napi_pp_put_page(page, napi_safe))
>
> Ah, it looks like we don't need is_pp_page() after all because
> napi_pp_put_page() already checks for that internally. Good find, much
> better than what I suggested.
>
> > return;
> > @@ -3464,6 +3462,12 @@ napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > put_page(page);
> > }
> >
> > +static inline void
> > +napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
> > +{
> > + napi_page_unref(skb_frag_page(frag), recycle, napi_safe);
> > +}
> > +
> > /**
> > * __skb_frag_unref - release a reference on a paged fragment.
> > * @frag: the paged fragment
> > diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> > index 4dd9e5040672..3126aeb0588d 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;
> > @@ -112,9 +112,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> > /* Unref skb_frag_pages in the src scatterlist if necessary.
> > * Skip the first sg which comes from skb->data.
> > */
> > - if (req->src != req->dst)
> > - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> > - put_page(sg_page(sg));
> > + if (req->src != req->dst) {
> > + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> > + struct page *page = sg_page(sg);
> > +
> > + napi_page_unref(page, skb->pp_recycle, false);
>
> [minor annoying nit, feel free to ignore]: I would use sg_page(sg)
> inline here and below, since page is not reused elsewhere. But I'm
> fine either way.
>
You are right. I will use sg_page(sg) directly.
> > + }
> > + }
> > }
> >
> > #ifdef CONFIG_INET_ESPINTCP
> > @@ -260,7 +264,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 +643,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..7bbe19e74cf0 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;
> > @@ -129,9 +129,13 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
> > /* Unref skb_frag_pages in the src scatterlist if necessary.
> > * Skip the first sg which comes from skb->data.
> > */
> > - if (req->src != req->dst)
> > - for (sg = sg_next(req->src); sg; sg = sg_next(sg))
> > - put_page(sg_page(sg));
> > + if (req->src != req->dst) {
> > + for (sg = sg_next(req->src); sg; sg = sg_next(sg)) {
> > + struct page *page = sg_page(sg);
> > +
> > + napi_page_unref(page, skb->pp_recycle, false);
> > + }
> > + }
> > }
> >
> > #ifdef CONFIG_INET6_ESPINTCP
> > @@ -294,7 +298,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 +681,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);
> > --
> > 2.42.0
> >
>
>