Re: [PATCH net v3] net: esp: fix bad handling of pages from page_pool

From: Ilias Apalodimas
Date: Mon Mar 11 2024 - 04:59:59 EST

Next message: Théo Lebrun: "Re: [PATCH v3 08/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller"
Previous message: Sui Jingfeng: "[PATCH v3 5/5] drm-bridge: sii902x: Use fwnode API to acquire device properties"
In reply to: Dragos Tatulea: "Re: [PATCH net v3] net: esp: fix bad handling of pages from page_pool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dragos

On Mon, 11 Mar 2024 at 10:51, Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
>
> Hi Ilias,
>
> On Mon, 2024-03-11 at 10:43 +0200, Ilias Apalodimas wrote:
> > Hi Dragos,
> >
> > On Fri, 8 Mar 2024 at 17:27, Dragos Tatulea <dtatulea@xxxxxxxxxx> 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.
> > >
> >
> > Indeed if you call put_page, you will leak a page. But won't that skb
> > eventually be freed by skb_free_head()?
> >
> I don't think it will: if you look at esp_output_tail (!esp->inplace case), the
> skb frags are put on the sg list and then the skb is cut off to have a single
> frag (with a "regular" page from x->xfrag). So if the pages from the old
> fragments are not freed here they will leak altogether.

Ok, if they won't reach that function -- which calls napi_pp_put_page
itself, then this looks sane

Acked-by: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>

>
> Thanks,
> Dragos
>
> > Thanks
> > /Ilias
> >
> > > 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.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling")
> > > Reported-and-tested-by: Anatoli N.Chechelnickiy <Anatoli.Chechelnickiy@xxxxxxxxxxxxxxx>
> > > Reported-by: Ian Kumlien <ian.kumlien@xxxxxxxxx>
> > > Link: https://lore.kernel.org/netdev/CAA85sZvvHtrpTQRqdaOx6gd55zPAVsqMYk_Lwh4Md5knTq7AyA@xxxxxxxxxxxxxx
> > > Signed-off-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> > > Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> > > Reviewed-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > > - Fixes in tags.
> > >
> > > Changes in v3:
> > > - Rebased to ipsec tree.
> > > ---
> > > 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 2dde34c29203..d9a1ccfb5708 100644
> > > --- a/include/linux/skbuff.h
> > > +++ b/include/linux/skbuff.h
> > > @@ -3448,6 +3448,16 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f)
> > >
> > > 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);
> > > --
> > > 2.42.0
> > >
>

Next message: Théo Lebrun: "Re: [PATCH v3 08/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller"
Previous message: Sui Jingfeng: "[PATCH v3 5/5] drm-bridge: sii902x: Use fwnode API to acquire device properties"
In reply to: Dragos Tatulea: "Re: [PATCH net v3] net: esp: fix bad handling of pages from page_pool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]