From: Alexander Duyck <firstname.lastname@example.org> To: Yunsheng Lin <email@example.com>, Jean-Philippe Brucker <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org> Cc: "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org> Subject: RE: [PATCH net] skbuff: disable coalescing for page_pool recycling Date: Fri, 25 Mar 2022 02:50:46 +0000 [thread overview] Message-ID: <SA1PR15MB5137A34F08A624A565150338BD1A9@SA1PR15MB5137.namprd15.prod.outlook.com> (raw) In-Reply-To: <email@example.com> > -----Original Message----- > From: Yunsheng Lin <firstname.lastname@example.org> > Sent: Thursday, March 24, 2022 7:00 PM > To: Jean-Philippe Brucker <email@example.com>; > firstname.lastname@example.org; email@example.com > Cc: firstname.lastname@example.org; email@example.com; firstname.lastname@example.org; > email@example.com; Alexander Duyck <firstname.lastname@example.org> > Subject: Re: [PATCH net] skbuff: disable coalescing for page_pool recycling > > +cc Alexander Duyck > > On 2022/3/25 1:29, Jean-Philippe Brucker wrote: > > Fix a use-after-free when using page_pool with page fragments. We > > encountered this problem during normal RX in the hns3 driver: > > > > (1) Initially we have three descriptors in the RX queue. The first one > > allocates PAGE1 through page_pool, and the other two allocate one > > half of PAGE2 each. Page references look like this: > > > > RX_BD1 _______ PAGE1 > > RX_BD2 _______ PAGE2 > > RX_BD3 _________/ > > > > (2) Handle RX on the first descriptor. Allocate SKB1, eventually added > > to the receive queue by tcp_queue_rcv(). > > > > (3) Handle RX on the second descriptor. Allocate SKB2 and pass it to > > netif_receive_skb(): > > > > netif_receive_skb(SKB2) > > ip_rcv(SKB2) > > SKB3 = skb_clone(SKB2) > > > > SKB2 and SKB3 share a reference to PAGE2 through > > skb_shinfo()->dataref. The other ref to PAGE2 is still held by > > RX_BD3: > > > > SKB2 ---+- PAGE2 > > SKB3 __/ / > > RX_BD3 _________/ > > > > (3b) Now while handling TCP, coalesce SKB3 with SKB1: > > > > tcp_v4_rcv(SKB3) > > tcp_try_coalesce(to=SKB1, from=SKB3) // succeeds > > kfree_skb_partial(SKB3) > > skb_release_data(SKB3) // drops one dataref > > > > SKB1 _____ PAGE1 > > \____ > > SKB2 _____ PAGE2 > > / > > RX_BD3 _________/ > > > > The problem is here: both SKB1 and SKB2 point to PAGE2 but SKB1 does > > not actually hold a reference to PAGE2. > > it seems the SKB1 *does* hold a reference to PAGE2 by calling > __skb_frag_ref(), which increments the page->_refcount instead of > incrementing pp_frag_count, as skb_cloned(SKB3) is true and > __skb_frag_ref() does not handle page pool > case: > > INVALID URI REMOVED > rc1/source/net/core/skbuff.c*L5308__;Iw!!Bt8RZUm9aw!u944ZiA7uzBuFvccr > rtR1xvondLNnkMf5xzM8xbbkosow-v5t-XdZJd6bMsByMx2Kw$ I'm confused here as well. I don't see a path where you can take ownership of the page without taking a reference. Specifically the skb_head_is_locked() won't let you steal the head if the skb is cloned. And then for the frags they have an additional reference taken if the skb is cloned. > Without coalescing, when > > releasing both SKB2 and SKB3, a single reference to PAGE2 would be > > dropped. Now when releasing SKB1 and SKB2, two references to PAGE2 > > will be dropped, resulting in underflow. > > > > (3c) Drop SKB2: > > > > af_packet_rcv(SKB2) > > consume_skb(SKB2) > > skb_release_data(SKB2) // drops second dataref > > page_pool_return_skb_page(PAGE2) // drops one pp_frag_count > > > > SKB1 _____ PAGE1 > > \____ > > PAGE2 > > / > > RX_BD3 _________/ > > > > (4) Userspace calls recvmsg() > > Copies SKB1 and releases it. Since SKB3 was coalesced with SKB1, we > > release the SKB3 page as well: > > > > tcp_eat_recv_skb(SKB1) > > skb_release_data(SKB1) > > page_pool_return_skb_page(PAGE1) > > page_pool_return_skb_page(PAGE2) // drops second > pp_frag_count > > > > (5) PAGE2 is freed, but the third RX descriptor was still using it! > > In our case this causes IOMMU faults, but it would silently corrupt > > memory if the IOMMU was disabled. I think I see the problem. It is when you get into steps 4 and 5 that you are actually hitting the issue. When you coalesced the page you ended up switching the page from a page pool page to a reference counted page, but it is being stored in a page pool skb. That is the issue. Basically if the skb is a pp_recycle skb we should be incrementing the frag count, not the reference count. So essentially the logic should be that if to->pp_recycle is set but from is cloned then you need to return false. The problem isn't that they are both pp_recycle skbs, it is that the from was cloned and we are trying to merge that into a pp_recycle skb by adding to the reference count of the pages. > > A proper implementation would probably take another reference from the > > page_pool at step (3b), but that seems too complicated for a fix. Keep > > it simple for now, prevent coalescing for page_pool users.
next prev parent reply other threads:[~2022-03-25 2:50 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-24 17:29 Jean-Philippe Brucker 2022-03-25 1:59 ` Yunsheng Lin 2022-03-25 2:50 ` Alexander Duyck [this message] 2022-03-28 12:02 ` Jean-Philippe Brucker 2022-03-28 12:18 ` Ilias Apalodimas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=SA1PR15MB5137A34F08A624A565150338BD1A9@SA1PR15MB5137.namprd15.prod.outlook.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='RE: [PATCH net] skbuff: disable coalescing for page_pool recycling' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).