All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] skbuff: Fix a potential race while recycling page_pool packets
@ 2021-07-08 16:24 Ilias Apalodimas
  2021-07-08 16:30 ` Ilias Apalodimas
  0 siblings, 1 reply; 3+ messages in thread
From: Ilias Apalodimas @ 2021-07-08 16:24 UTC (permalink / raw)
  To: netdev
  Cc: Ilias Apalodimas, Alexander Duyck, David S. Miller,
	Jakub Kicinski, Alexander Lobakin, Jonathan Lemon,
	Willem de Bruijn, Miaohe Lin, Guillaume Nault, Cong Wang,
	Jesper Dangaard Brouer, Matteo Croce, linux-kernel

As Alexander points out, when we are trying to recycle a cloned/expanded
SKB we might trigger a race.  The recycling code relies on the
pp_recycle bit to trigger,  which we carry that over to cloned SKBs.
When that cloned SKB gets expanded,  we are creating 2 separate instances
accessing the page frags.  Since the skb_release_data() will first try to
recycle the frags,  there's a potential race between the original and
cloned SKB.

Fix this by explicitly making the cloned/expanded SKB not recyclable.
If the original SKB is freed first the pages are released.
If it is released after the clone/expended skb then it can still be
recycled.

Fixes: 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling")
Reported-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 net/core/skbuff.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 12aabcda6db2..0cb53c05ed76 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1718,6 +1718,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	}
 	off = (data + nhead) - skb->head;
 
+	/* If it's a cloned skb we expand with frags attached we must prohibit
+	 * the recycling code from running, otherwise we might trigger a race
+	 * while trying to recycle the fragments from the original and cloned
+	 * skb
+	 */
+	if (skb_cloned(skb))
+		skb->pp_recycle = 0;
 	skb->head     = data;
 	skb->head_frag = 0;
 	skb->data    += off;
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] skbuff: Fix a potential race while recycling page_pool packets
  2021-07-08 16:24 [PATCH] skbuff: Fix a potential race while recycling page_pool packets Ilias Apalodimas
@ 2021-07-08 16:30 ` Ilias Apalodimas
  2021-07-08 16:32   ` Alexander Duyck
  0 siblings, 1 reply; 3+ messages in thread
From: Ilias Apalodimas @ 2021-07-08 16:30 UTC (permalink / raw)
  To: Networking, Alexander Duyck
  Cc: David S. Miller, Jakub Kicinski, Alexander Lobakin,
	Jonathan Lemon, Willem de Bruijn, Miaohe Lin, Guillaume Nault,
	Cong Wang, Jesper Dangaard Brouer, Matteo Croce, open list

+cc Alexander on his gmail address since the Intel one bounced.

Alexander want me to respin it with you gmail address on the Reported-by?

Sorry for the noise
/Ilias

On Thu, 8 Jul 2021 at 19:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> As Alexander points out, when we are trying to recycle a cloned/expanded
> SKB we might trigger a race.  The recycling code relies on the
> pp_recycle bit to trigger,  which we carry that over to cloned SKBs.
> When that cloned SKB gets expanded,  we are creating 2 separate instances
> accessing the page frags.  Since the skb_release_data() will first try to
> recycle the frags,  there's a potential race between the original and
> cloned SKB.
>
> Fix this by explicitly making the cloned/expanded SKB not recyclable.
> If the original SKB is freed first the pages are released.
> If it is released after the clone/expended skb then it can still be
> recycled.
>
> Fixes: 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling")
> Reported-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  net/core/skbuff.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 12aabcda6db2..0cb53c05ed76 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1718,6 +1718,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>         }
>         off = (data + nhead) - skb->head;
>
> +       /* If it's a cloned skb we expand with frags attached we must prohibit
> +        * the recycling code from running, otherwise we might trigger a race
> +        * while trying to recycle the fragments from the original and cloned
> +        * skb
> +        */
> +       if (skb_cloned(skb))
> +               skb->pp_recycle = 0;
>         skb->head     = data;
>         skb->head_frag = 0;
>         skb->data    += off;
> --
> 2.32.0.rc0
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] skbuff: Fix a potential race while recycling page_pool packets
  2021-07-08 16:30 ` Ilias Apalodimas
@ 2021-07-08 16:32   ` Alexander Duyck
  0 siblings, 0 replies; 3+ messages in thread
From: Alexander Duyck @ 2021-07-08 16:32 UTC (permalink / raw)
  To: Ilias Apalodimas
  Cc: Networking, David S. Miller, Jakub Kicinski, Alexander Lobakin,
	Jonathan Lemon, Willem de Bruijn, Miaohe Lin, Guillaume Nault,
	Cong Wang, Jesper Dangaard Brouer, Matteo Croce, open list

On Thu, Jul 8, 2021 at 9:31 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> +cc Alexander on his gmail address since the Intel one bounced.
>
> Alexander want me to respin it with you gmail address on the Reported-by?
>
> Sorry for the noise
> /Ilias
>
> On Thu, 8 Jul 2021 at 19:24, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > As Alexander points out, when we are trying to recycle a cloned/expanded
> > SKB we might trigger a race.  The recycling code relies on the
> > pp_recycle bit to trigger,  which we carry that over to cloned SKBs.
> > When that cloned SKB gets expanded,  we are creating 2 separate instances
> > accessing the page frags.  Since the skb_release_data() will first try to
> > recycle the frags,  there's a potential race between the original and
> > cloned SKB.
> >
> > Fix this by explicitly making the cloned/expanded SKB not recyclable.
> > If the original SKB is freed first the pages are released.
> > If it is released after the clone/expended skb then it can still be
> > recycled.
> >
> > Fixes: 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling")
> > Reported-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  net/core/skbuff.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 12aabcda6db2..0cb53c05ed76 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -1718,6 +1718,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> >         }
> >         off = (data + nhead) - skb->head;
> >
> > +       /* If it's a cloned skb we expand with frags attached we must prohibit
> > +        * the recycling code from running, otherwise we might trigger a race
> > +        * while trying to recycle the fragments from the original and cloned
> > +        * skb
> > +        */
> > +       if (skb_cloned(skb))
> > +               skb->pp_recycle = 0;
> >         skb->head     = data;
> >         skb->head_frag = 0;
> >         skb->data    += off;

Yeah, I would recommend a respin.

Also I would move this line up to the skb_cloned block just a few
lines before this spot just to avoid a second check.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-07-08 16:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08 16:24 [PATCH] skbuff: Fix a potential race while recycling page_pool packets Ilias Apalodimas
2021-07-08 16:30 ` Ilias Apalodimas
2021-07-08 16:32   ` Alexander Duyck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.