[Intel-wired-lan] [PATCH] igb: avoid premature Rx buffer reuse
Alexander Duyck
alexander.duyck at gmail.com
Tue Jan 12 21:22:41 UTC 2021
On Mon, Jan 11, 2021 at 6:54 PM Li,Rongqing <lirongqing at baidu.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
> > Sent: Tuesday, January 12, 2021 4:54 AM
> > To: Li,Rongqing <lirongqing at baidu.com>
> > Cc: Netdev <netdev at vger.kernel.org>; intel-wired-lan
> > <intel-wired-lan at lists.osuosl.org>; Björn Töpel <bjorn.topel at intel.com>
> > Subject: Re: [PATCH] igb: avoid premature Rx buffer reuse
> >
> > On Wed, Jan 6, 2021 at 7:53 PM Li RongQing <lirongqing at baidu.com> wrote:
> > >
> > > The page recycle code, incorrectly, relied on that a page fragment
> > > could not be freed inside xdp_do_redirect(). This assumption leads to
> > > that page fragments that are used by the stack/XDP redirect can be
> > > reused and overwritten.
> > >
> > > To avoid this, store the page count prior invoking xdp_do_redirect().
> > >
> > > Fixes: 9cbc948b5a20 ("igb: add XDP support")
> > > Signed-off-by: Li RongQing <lirongqing at baidu.com>
> > > Cc: Björn Töpel <bjorn.topel at intel.com>
> >
> > I'm not sure what you are talking about here. We allow for a 0 to 1 count
> > difference in the pagecount bias. The idea is the driver should be holding onto
> > at least one reference from the driver at all times.
> > Are you saying that is not the case?
> >
> > As far as the code itself we hold onto the page as long as our difference does
> > not exceed 1. So specifically if the XDP call is freeing the page the page itself
> > should still be valid as the reference count shouldn't drop below 1, and in that
> > case the driver should be holding that one reference to the page.
> >
> > When we perform our check we are performing it such at output of either 0 if
> > the page is freed, or 1 if the page is not freed are acceptable for us to allow
> > reuse. The key bit is in igb_clean_rx_irq where we will flip the buffer for the
> > IGB_XDP_TX | IGB_XDP_REDIR case and just increment the pagecnt_bias
> > indicating that the page was dropped in the non-flipped case.
> >
> > Are you perhaps seeing a function that is returning an error and still consuming
> > the page? If so that might explain what you are seeing.
> > However the bug would be in the other driver not this one. The
> > xdp_do_redirect function is not supposed to free the page if it returns an error.
> > It is supposed to leave that up to the function that called xdp_do_redirect.
> >
> > > ---
> > > drivers/net/ethernet/intel/igb/igb_main.c | 22 +++++++++++++++-------
> > > 1 file changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> > > b/drivers/net/ethernet/intel/igb/igb_main.c
> > > index 03f78fdb0dcd..3e0d903cf919 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > > @@ -8232,7 +8232,8 @@ static inline bool igb_page_is_reserved(struct
> > page *page)
> > > return (page_to_nid(page) != numa_mem_id()) ||
> > > page_is_pfmemalloc(page); }
> > >
> > > -static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
> > > +static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer,
> > > +
> > int
> > > +rx_buf_pgcnt)
> > > {
> > > unsigned int pagecnt_bias = rx_buffer->pagecnt_bias;
> > > struct page *page = rx_buffer->page; @@ -8243,7 +8244,7 @@
> > > static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
> > >
> > > #if (PAGE_SIZE < 8192)
> > > /* if we are only owner of page we can reuse it */
> > > - if (unlikely((page_ref_count(page) - pagecnt_bias) > 1))
> > > + if (unlikely((rx_buf_pgcnt - pagecnt_bias) > 1))
> > > return false;
> > > #else
> > I would need more info on the actual issue. If nothing else it might be useful to
> > have an example where you print out the page_ref_count versus the
> > pagecnt_bias at a few points to verify exactly what is going on. As I said before
> > if the issue is the xdp_do_redirect returning an error and still consuming the
> > page then the bug is elsewhere and not here.
>
>
> This patch is same as 75aab4e10ae6a4593a60f66d13de755d4e91f400
>
>
> commit 75aab4e10ae6a4593a60f66d13de755d4e91f400
> Author: Björn Töpel <bjorn.topel at intel.com>
> Date: Tue Aug 25 19:27:34 2020 +0200
>
> i40e: avoid premature Rx buffer reuse
>
> The page recycle code, incorrectly, relied on that a page fragment
> could not be freed inside xdp_do_redirect(). This assumption leads to
> that page fragments that are used by the stack/XDP redirect can be
> reused and overwritten.
>
> To avoid this, store the page count prior invoking xdp_do_redirect().
>
> Longer explanation:
>
> Intel NICs have a recycle mechanism. The main idea is that a page is
> split into two parts. One part is owned by the driver, one part might
> be owned by someone else, such as the stack.
>
> t0: Page is allocated, and put on the Rx ring
> +---------------
> used by NIC ->| upper buffer
> (rx_buffer) +---------------
> | lower buffer
> +---------------
> page count == USHRT_MAX
> rx_buffer->pagecnt_bias == USHRT_MAX
>
> t1: Buffer is received, and passed to the stack (e.g.)
> +---------------
> | upper buff (skb)
> +---------------
> used by NIC ->| lower buffer
> (rx_buffer) +---------------
> page count == USHRT_MAX
> rx_buffer->pagecnt_bias == USHRT_MAX - 1
> t2: Buffer is received, and redirected
> +---------------
> | upper buff (skb)
> +---------------
> used by NIC ->| lower buffer
> (rx_buffer) +---------------
>
> Now, prior calling xdp_do_redirect():
> page count == USHRT_MAX
> rx_buffer->pagecnt_bias == USHRT_MAX - 2
>
> This means that buffer *cannot* be flipped/reused, because the skb is
> still using it.
>
> The problem arises when xdp_do_redirect() actually frees the
> segment. Then we get:
> page count == USHRT_MAX - 1
> rx_buffer->pagecnt_bias == USHRT_MAX - 2
>
> From a recycle perspective, the buffer can be flipped and reused,
> which means that the skb data area is passed to the Rx HW ring!
>
> To work around this, the page count is stored prior calling
> xdp_do_redirect().
>
> Note that this is not optimal, since the NIC could actually reuse the
> "lower buffer" again. However, then we need to track whether
> XDP_REDIRECT consumed the buffer or not.
>
> Fixes: d9314c474d4f ("i40e: add support for XDP_REDIRECT")
> Reported-and-analyzed-by: Li RongQing <lirongqing at baidu.com>
> Signed-off-by: Björn Töpel <bjorn.topel at intel.com>
> Tested-by: George Kuruvinakunnel <george.kuruvinakunnel at intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen at intel.com>
>
>
> Thanks
>
> -Li
Okay, this explanation makes much more sense. Could you please either
include this explanation in your patch, or include a reference to this
patch as this explains clearly what the issue is while yours didn't
and led to the confusion as I was assuming the freeing was happening
closer to the t0 case, and really the problem is t1.
Thanks.
- Alex
More information about the Intel-wired-lan
mailing list