Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing@baidu.com> wrote:
>
> This fixes ice/i40e/ixgbe/ixgbevf_rx_buffer_flip in
> copy mode xdp that can lead to data corruption.
>
> I split two patches, since i40e/xgbe/ixgbevf supports xsk
> receiving from 4.18, put their fixes in a patch
>

Li, sorry for the looong latency. I took a looong vacation. :-P

Thanks for taking a look at this, but I believe this is not a bug.

The Intel Ethernet drivers (obviously non-zerocopy AF_XDP -- "good ol'
XDP") use a page reuse algorithm.

Basic idea is that a page is allocated from the page allocator
(i40e_alloc_mapped_page()). The refcount is increased to
USHRT_MAX. The page is split into two chunks (simplified). If there's
one user of the page, the page can be reused (flipped). If not, a new
page needs to be allocated (with the large refcount).

So, the idea is that usually the page can be reused (flipped), and the
page only needs to be "put" not "get" since the refcount was initally
bumped to a large value.

All frames (except XDP_DROP which can be reused directly) "die" via
page_frag_free() which decreases the page refcount, and frees the page
if the refcount is zero.

Let's take some scenarios as examples:

1. A frame is received in "vanilla" XDP (MEM_TYPE_PAGE_SHARED), and
   the XDP program verdict is XDP_TX. The frame will be placed on the
   HW Tx ring, and freed* (async) in i40e_clean_tx_irq:
        /* free the skb/XDP data */
        if (ring_is_xdp(tx_ring))
            xdp_return_frame(tx_buf->xdpf); // calls page_frag_free()

2. A frame is passed to the stack, eventually it's freed* via
   skb_free_frag().

3. A frame is passed to an AF_XDP socket. The data is copied to the
   socket data area, and the frame is directly freed*.

Not the * by the freed. Actually freeing here means calling
page_frag_free(), which means decreasing the refcount. The page reuse
algorithm makes sure that the buffers are not stale.

The only difference from XDP_TX and XDP_DIRECT to dev/cpumaps,
compared to AF_XDP sockets is that the latter calls page_frag_free()
directly, whereas the other does it asynchronous from the Tx clean up
phase.

Let me know if it's still not clear, but the bottom line is that none
of these patches are needed.


Thanks!
Björn


> Li RongQing (2):
>   xdp: i40e: ixgbe: ixgbevf: not flip rx buffer for copy mode xdp
>   ice/xdp: not adjust rx buffer for copy mode xdp
>
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 5 ++++-
>  drivers/net/ethernet/intel/ice/ice_txrx.c         | 5 ++++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 5 ++++-
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 ++++-
>  include/net/xdp.h                                 | 3 +++
>  net/xdp/xsk.c                                     | 4 +++-
>  6 files changed, 22 insertions(+), 5 deletions(-)
>
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Björn Töpel [mailto:bjorn.topel@gmail.com]
> 发送时间: 2020年8月18日 22:05
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: Netdev <netdev@vger.kernel.org>; intel-wired-lan
> <intel-wired-lan@lists.osuosl.org>; Karlsson, Magnus
> <magnus.karlsson@intel.com>; Björn Töpel <bjorn.topel@intel.com>; bpf
> <bpf@vger.kernel.org>; Maciej Fijalkowski <maciej.fijalkowski@intel.com>;
> Piotr <piotr.raczynski@intel.com>; Maciej <maciej.machnikowski@intel.com>
> 主题: Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer
> 
> On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing@baidu.com> wrote:
> >
> > This fixes ice/i40e/ixgbe/ixgbevf_rx_buffer_flip in copy mode xdp that
> > can lead to data corruption.
> >
> > I split two patches, since i40e/xgbe/ixgbevf supports xsk receiving
> > from 4.18, put their fixes in a patch
> >
> 
> Li, sorry for the looong latency. I took a looong vacation. :-P
> 
> Thanks for taking a look at this, but I believe this is not a bug.
> 
> The Intel Ethernet drivers (obviously non-zerocopy AF_XDP -- "good ol'
> XDP") use a page reuse algorithm.
> 
> Basic idea is that a page is allocated from the page allocator
> (i40e_alloc_mapped_page()). The refcount is increased to USHRT_MAX. The
> page is split into two chunks (simplified). If there's one user of the page, the
> page can be reused (flipped). If not, a new page needs to be allocated (with the
> large refcount).
> 
> So, the idea is that usually the page can be reused (flipped), and the page only
> needs to be "put" not "get" since the refcount was initally bumped to a large
> value.
> 
> All frames (except XDP_DROP which can be reused directly) "die" via
> page_frag_free() which decreases the page refcount, and frees the page if the
> refcount is zero.
> 
> Let's take some scenarios as examples:
> 
> 1. A frame is received in "vanilla" XDP (MEM_TYPE_PAGE_SHARED), and
>    the XDP program verdict is XDP_TX. The frame will be placed on the
>    HW Tx ring, and freed* (async) in i40e_clean_tx_irq:
>         /* free the skb/XDP data */
>         if (ring_is_xdp(tx_ring))
>             xdp_return_frame(tx_buf->xdpf); // calls page_frag_free()
> 
> 2. A frame is passed to the stack, eventually it's freed* via
>    skb_free_frag().
> 
> 3. A frame is passed to an AF_XDP socket. The data is copied to the
>    socket data area, and the frame is directly freed*.
> 
> Not the * by the freed. Actually freeing here means calling page_frag_free(),
> which means decreasing the refcount. The page reuse algorithm makes sure
> that the buffers are not stale.
> 
> The only difference from XDP_TX and XDP_DIRECT to dev/cpumaps, compared
> to AF_XDP sockets is that the latter calls page_frag_free() directly, whereas
> the other does it asynchronous from the Tx clean up phase.
> 

Hi:

Thanks for your explanation.

But we can reproduce this bug

We use ebpf to redirect only-Vxlan packets to non-zerocopy AF_XDP,  First we see panic on tcp stack, in tcp_collapse: BUG_ON(offset < 0); it is very hard to reproduce.

Then we use the scp to do test, and has lots of vxlan packet at the same time, scp will be broken frequently.

With this fixes, scp has not been broken again, and kernel is not panic again

Seem your explanation is unable to solve my analysis:

       1. first skb is not for xsk, and forwarded to another device
          or socket queue
       2. seconds skb is for xsk, copy data to xsk memory, and page
          of skb->data is released
       3. rx_buff is reusable since only first skb is in it, but
          *_rx_buffer_flip will make that page_offset is set to
          first skb data
       4. then reuse rx buffer, first skb which still is living
          will be corrupted.


The root cause is difference you said upper, so I only fixes for non-zerocopy AF_XDP

-Li
> Let me know if it's still not clear, but the bottom line is that none of these
> patches are needed.
> 
> 
> Thanks!
> Björn
> 
> 
> > Li RongQing (2):
> >   xdp: i40e: ixgbe: ixgbevf: not flip rx buffer for copy mode xdp
> >   ice/xdp: not adjust rx buffer for copy mode xdp
> >
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 5 ++++-
> >  drivers/net/ethernet/intel/ice/ice_txrx.c         | 5 ++++-
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 5 ++++-
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 ++++-
> >  include/net/xdp.h                                 | 3 +++
> >  net/xdp/xsk.c                                     | 4 +++-
> >  6 files changed, 22 insertions(+), 5 deletions(-)
> >
> > --
> > 2.16.2
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On 2020-08-19 03:37, Li,Rongqing wrote:
[...]
 > Hi:
 >
 > Thanks for your explanation.
 >
 > But we can reproduce this bug
 >
 > We use ebpf to redirect only-Vxlan packets to non-zerocopy AF_XDP, 
First we see panic on tcp stack, in tcp_collapse: BUG_ON(offset < 0); it 
is very hard to reproduce.
 >
 > Then we use the scp to do test, and has lots of vxlan packet at the 
same time, scp will be broken frequently.
 >

Ok! Just so that I'm certain of your setup. You receive packets to an
i40e netdev where there's an XDP program. The program does XDP_PASS or
XDP_REDIRECT to e.g. devmap for non-vxlan packets. However, vxlan
packets are redirected to AF_XDP socket(s) in *copy-mode*. Am I
understanding that correct?

I'm assuming this is an x86-64 with 4k page size, right? :-) The page
flipping is a bit different if the PAGE_SIZE is not 4k.

 > With this fixes, scp has not been broken again, and kernel is not 
panic again
 >

Let's dig into your scenario.

Are you saying the following:

Page A:
+------------
| "first skb" ----> Rx HW ring entry X
+------------
| "second skb"----> Rx HW ring entry X+1 (or X+n)
+------------

This is a scenario that shouldn't be allowed, because there are now
two users of the page. If that's the case, the refcounting is
broken. Is that the case?

Check out i40e_can_reuse_rx_page(). The idea with page flipping/reuse
is that the page is only reused if there is only one user.

 > Seem your explanation is unable to solve my analysis:
 >
 >         1. first skb is not for xsk, and forwarded to another device
 >            or socket queue

The data for the "first skb" resides on a page:
A:
+------------
| "first skb"
+------------
| to be reused
+------------
refcount >>1

 >         2. seconds skb is for xsk, copy data to xsk memory, and page
 >            of skb->data is released

Note that page B != page A.

B:
+------------
| to be reused/or used by the stack
+------------
| "second skb for xsk"
+------------
refcount >>1

data is copied to socket, page_frag_free() is called, and the page
count is decreased. The driver will then check if the page can be
reused. If not, it's freed to the page allocator.

 >         3. rx_buff is reusable since only first skb is in it, but
 >            *_rx_buffer_flip will make that page_offset is set to
 >            first skb data

I'm having trouble grasping how this is possible. More than one user
implies that it wont be reused. If this is possible, the
recounting/reuse mechanism is broken, and that is what should be
fixed.

The AF_XDP redirect should not have semantics different from, say,
devmap redirect. It's just that the page_frag_free() is called earlier
for AF_XDP, instead of from i40e_clean_tx_irq() as the case for
devmap/XDP_TX.

 >         4. then reuse rx buffer, first skb which still is living
 >            will be corrupted.
 >
 >
 > The root cause is difference you said upper, so I only fixes for 
non-zerocopy AF_XDP
 >

I have only addressed non-zerocopy, so we're on the same page (pun
intended) here!


Björn

 > -Li

答复: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Björn Töpel [mailto:bjorn.topel@intel.com]
> 发送时间: 2020年8月19日 14:45
> 收件人: Li,Rongqing <lirongqing@baidu.com>; Björn Töpel
> <bjorn.topel@gmail.com>
> 抄送: Netdev <netdev@vger.kernel.org>; intel-wired-lan
> <intel-wired-lan@lists.osuosl.org>; Karlsson, Magnus
> <magnus.karlsson@intel.com>; bpf <bpf@vger.kernel.org>; Maciej Fijalkowski
> <maciej.fijalkowski@intel.com>; Piotr <piotr.raczynski@intel.com>; Maciej
> <maciej.machnikowski@intel.com>
> 主题: Re: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer
> 
> On 2020-08-19 03:37, Li,Rongqing wrote:
> [...]
>  > Hi:
>  >
>  > Thanks for your explanation.
>  >
>  > But we can reproduce this bug
>  >
>  > We use ebpf to redirect only-Vxlan packets to non-zerocopy AF_XDP, First we
> see panic on tcp stack, in tcp_collapse: BUG_ON(offset < 0); it is very hard to
> reproduce.
>  >
>  > Then we use the scp to do test, and has lots of vxlan packet at the same
> time, scp will be broken frequently.
>  >
> 
> Ok! Just so that I'm certain of your setup. You receive packets to an i40e netdev
> where there's an XDP program. The program does XDP_PASS or XDP_REDIRECT
> to e.g. devmap for non-vxlan packets. However, vxlan packets are redirected to
> AF_XDP socket(s) in *copy-mode*. Am I understanding that correct?
> 
Similar as your description, 

but the xdp program only redirects vxlan packets to af_xdp socket, other packets will go to Linux kernel networking stack, like scp/ssh packets


> I'm assuming this is an x86-64 with 4k page size, right? :-) The page flipping is a
> bit different if the PAGE_SIZE is not 4k.
> 

We use 4k page size, page flipping is 4k, we did not change the i40e drivers, 4.19 stable kernel

>  > With this fixes, scp has not been broken again, and kernel is not panic
> again  >
> 
> Let's dig into your scenario.
> 
> Are you saying the following:
> 
> Page A:
> +------------
> | "first skb" ----> Rx HW ring entry X 
> +------------
> | "second skb"----> Rx HW ring entry X+1 (or X+n)
> +------------
> 

Like:

First skb will be into tcp socket rx queue

Seconds skb is vxlan packet, will be copy to af_xdp socket, and released.

> This is a scenario that shouldn't be allowed, because there are now two users
> of the page. If that's the case, the refcounting is broken. Is that the case?
> 

True, it is broken for copy mode xsk

-Li

> Check out i40e_can_reuse_rx_page(). The idea with page flipping/reuse is that
> the page is only reused if there is only one user.
> 
>  > Seem your explanation is unable to solve my analysis:
>  >
>  >         1. first skb is not for xsk, and forwarded to another device
>  >            or socket queue
> 
> The data for the "first skb" resides on a page:
> A:
> +------------
> | "first skb"
> +------------
> | to be reused
> +------------
> refcount >>1
> 
>  >         2. seconds skb is for xsk, copy data to xsk memory, and page
>  >            of skb->data is released
> 
> Note that page B != page A.
> 
> B:
> +------------
> | to be reused/or used by the stack
> +------------
> | "second skb for xsk"
> +------------
> refcount >>1
> 
> data is copied to socket, page_frag_free() is called, and the page count is
> decreased. The driver will then check if the page can be reused. If not, it's freed
> to the page allocator.
> 
>  >         3. rx_buff is reusable since only first skb is in it, but
>  >            *_rx_buffer_flip will make that page_offset is set to
>  >            first skb data
> 
> I'm having trouble grasping how this is possible. More than one user implies
> that it wont be reused. If this is possible, the recounting/reuse mechanism is
> broken, and that is what should be fixed.
> 
> The AF_XDP redirect should not have semantics different from, say, devmap
> redirect. It's just that the page_frag_free() is called earlier for AF_XDP, instead
> of from i40e_clean_tx_irq() as the case for devmap/XDP_TX.
> 
>  >         4. then reuse rx buffer, first skb which still is living
>  >            will be corrupted.
>  >
>  >
>  > The root cause is difference you said upper, so I only fixes for non-zerocopy
> AF_XDP  >
> 
> I have only addressed non-zerocopy, so we're on the same page (pun
> intended) here!
> 
> 
> Björn
> 
>  > -Li

Re: 答复: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On 2020-08-19 10:17, Li,Rongqing wrote:
> 
> 
>> -----邮件原件-----
>> 发件人: Björn Töpel [mailto:bjorn.topel@intel.com]
>> 发送时间: 2020年8月19日 14:45
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; Björn Töpel
>> <bjorn.topel@gmail.com>
>> 抄送: Netdev <netdev@vger.kernel.org>; intel-wired-lan
>> <intel-wired-lan@lists.osuosl.org>; Karlsson, Magnus
>> <magnus.karlsson@intel.com>; bpf <bpf@vger.kernel.org>; Maciej Fijalkowski
>> <maciej.fijalkowski@intel.com>; Piotr <piotr.raczynski@intel.com>; Maciej
>> <maciej.machnikowski@intel.com>
>> 主题: Re: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer
>>
>> On 2020-08-19 03:37, Li,Rongqing wrote:
>> [...]
>>   > Hi:
>>   >
>>   > Thanks for your explanation.
>>   >
>>   > But we can reproduce this bug
>>   >
>>   > We use ebpf to redirect only-Vxlan packets to non-zerocopy AF_XDP, First we
>> see panic on tcp stack, in tcp_collapse: BUG_ON(offset < 0); it is very hard to
>> reproduce.
>>   >
>>   > Then we use the scp to do test, and has lots of vxlan packet at the same
>> time, scp will be broken frequently.
>>   >
>>
>> Ok! Just so that I'm certain of your setup. You receive packets to an i40e netdev
>> where there's an XDP program. The program does XDP_PASS or XDP_REDIRECT
>> to e.g. devmap for non-vxlan packets. However, vxlan packets are redirected to
>> AF_XDP socket(s) in *copy-mode*. Am I understanding that correct?
>>
> Similar as your description,
> 
> but the xdp program only redirects vxlan packets to af_xdp socket, other packets will go to Linux kernel networking stack, like scp/ssh packets
> 
> 
>> I'm assuming this is an x86-64 with 4k page size, right? :-) The page flipping is a
>> bit different if the PAGE_SIZE is not 4k.
>>
> 
> We use 4k page size, page flipping is 4k, we did not change the i40e drivers, 4.19 stable kernel
>

Would you mind testing on a newer kernel? Say the latest stable 5.8.2?


>>   > With this fixes, scp has not been broken again, and kernel is not panic
>> again  >
>>
>> Let's dig into your scenario.
>>
>> Are you saying the following:
>>
>> Page A:
>> +------------
>> | "first skb" ----> Rx HW ring entry X
>> +------------
>> | "second skb"----> Rx HW ring entry X+1 (or X+n)
>> +------------
>>
> 
> Like:
> 
> First skb will be into tcp socket rx queue
> 
> Seconds skb is vxlan packet, will be copy to af_xdp socket, and released.
> 
>> This is a scenario that shouldn't be allowed, because there are now two users
>> of the page. If that's the case, the refcounting is broken. Is that the case?
>>
> 
> True, it is broken for copy mode xsk
>

Ok. However, the fix is not avoiding the page_frag_free, but finding and 
fixing the refcount bug. I'll have a deeper look.

But please, try to reproduce with a newer kernel.


Thanks,
Björn

Re: 答复: 答复: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On 2020-08-19 10:31, Björn Töpel wrote:
[...]
> 
> But please, try to reproduce with a newer kernel.
>

Also, you are *sure* that you're touching stale data? Have you tried 
running with CONFIG_DEBUG_PAGEALLOC and CONFIG_PAGE_POISONING?


Björn

Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On Tue, 18 Aug 2020 at 16:04, Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing@baidu.com> wrote:
> >
> > This fixes ice/i40e/ixgbe/ixgbevf_rx_buffer_flip in
> > copy mode xdp that can lead to data corruption.
> >
> > I split two patches, since i40e/xgbe/ixgbevf supports xsk
> > receiving from 4.18, put their fixes in a patch
> >
>
> Li, sorry for the looong latency. I took a looong vacation. :-P
>
> Thanks for taking a look at this, but I believe this is not a bug.
>

Ok, dug a bit more into this. I had an offlist discussion with Li, and
there are two places (AFAIK) where Li experience a BUG() in
tcp_collapse():

            BUG_ON(offset < 0);
and
                if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
                    BUG();

(Li, please correct me if I'm wrong.)

I still claim that the page-flipping mechanism is correct, but I found
some weirdness in the build_skb() call.

In drivers/net/ethernet/intel/i40e/i40e_txrx.c, build_skb() is invoked as:
    skb = build_skb(xdp->data_hard_start, truesize);

For the setup Li has truesize is 2048 (half a page), but the
rx_buf_len is 1536. In the driver a packet is layed out as:

| padding 192 | packet data 1536 | skb shared info 320 |

build_skb() assumes that the second argument (frag_size) is max packet
size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)). In other words,
frag_size should not include the padding (192 above). In build_skb(),
frag_size is used to compute the skb truesize and skb end. i40e passes
a too large buffer, and can therefore potentially corrupt the skb, and
maybe this is the reason for tcp_collapse() splatting.

Li, could you test if you get the splat with this patch:

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 3e5c566ceb01..acfb4ad9b506 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2065,7 +2065,8 @@ static struct sk_buff *i40e_build_skb(struct
i40e_ring *rx_ring,
 {
     unsigned int metasize = xdp->data - xdp->data_meta;
 #if (PAGE_SIZE < 8192)
-    unsigned int truesize = i40e_rx_pg_size(rx_ring) / 2;
+    unsigned int truesize = rx_ring->rx_buf_len +
+                SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 #else
     unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
                 SKB_DATA_ALIGN(xdp->data_end -

I'll have a look in the other Intel drivers, and see if there are
similar issues. I'll cook a patch.


Björn

Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Maciej Fijalkowski]

On Thu, Aug 20, 2020 at 05:13:16PM +0200, Björn Töpel wrote:
> On Tue, 18 Aug 2020 at 16:04, Björn Töpel <bjorn.topel@gmail.com> wrote:
> >
> > On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing@baidu.com> wrote:
> > >
> > > This fixes ice/i40e/ixgbe/ixgbevf_rx_buffer_flip in
> > > copy mode xdp that can lead to data corruption.
> > >
> > > I split two patches, since i40e/xgbe/ixgbevf supports xsk
> > > receiving from 4.18, put their fixes in a patch
> > >
> >
> > Li, sorry for the looong latency. I took a looong vacation. :-P
> >
> > Thanks for taking a look at this, but I believe this is not a bug.
> >
> 
> Ok, dug a bit more into this. I had an offlist discussion with Li, and
> there are two places (AFAIK) where Li experience a BUG() in
> tcp_collapse():
> 
>             BUG_ON(offset < 0);
> and
>                 if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>                     BUG();
> 
> (Li, please correct me if I'm wrong.)
> 
> I still claim that the page-flipping mechanism is correct, but I found
> some weirdness in the build_skb() call.
> 
> In drivers/net/ethernet/intel/i40e/i40e_txrx.c, build_skb() is invoked as:
>     skb = build_skb(xdp->data_hard_start, truesize);
> 
> For the setup Li has truesize is 2048 (half a page), but the
> rx_buf_len is 1536. In the driver a packet is layed out as:
> 
> | padding 192 | packet data 1536 | skb shared info 320 |
> 
> build_skb() assumes that the second argument (frag_size) is max packet
> size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)). In other words,
> frag_size should not include the padding (192 above). In build_skb(),

Not sure I am buying that reasoning. It assumes the padding + packet_data
and we use skb_reserve() to tell the skb about the padding.

__build_skb_around() subtracts sizeof(struct skb_shared_info) from size
that we are providing, so now we are with padding + packet_data.
Then it is used to calculate the skb->end.

Back to i40e_build_skb(), we use the skb_reserve() to advance the
skb->data and skb->tail so that they point to packet_data. Finally
__skb_put() will move the skb->tail to the end of packet_data.

Wouldn't your approach disallow having the headroom at all in the linear
part of skb?

> frag_size is used to compute the skb truesize and skb end. i40e passes

IMHO skb->end is correct. For skb->truesize I would assume that the
headroom should also be taken into account for tracking how many bytes a
particular skb consumes, no?

> a too large buffer, and can therefore potentially corrupt the skb, and
> maybe this is the reason for tcp_collapse() splatting.
> 
> Li, could you test if you get the splat with this patch:
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 3e5c566ceb01..acfb4ad9b506 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2065,7 +2065,8 @@ static struct sk_buff *i40e_build_skb(struct
> i40e_ring *rx_ring,
>  {
>      unsigned int metasize = xdp->data - xdp->data_meta;
>  #if (PAGE_SIZE < 8192)
> -    unsigned int truesize = i40e_rx_pg_size(rx_ring) / 2;
> +    unsigned int truesize = rx_ring->rx_buf_len +
> +                SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

This will actually break the page flipping scheme. We need a separate
variable for that and use the old truesize to bump the page_offset.

>  #else
>      unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
>                  SKB_DATA_ALIGN(xdp->data_end -
> 
> I'll have a look in the other Intel drivers, and see if there are
> similar issues. I'll cook a patch.
> 
> 
> Björn

Re: [Intel-wired-lan] [PATCH 0/2] intel/xdp fixes for fliping rx buffer

[Björn Töpel]

On 2020-08-20 18:51, Maciej Fijalkowski wrote:
> On Thu, Aug 20, 2020 at 05:13:16PM +0200, Björn Töpel wrote:
>> On Tue, 18 Aug 2020 at 16:04, Björn Töpel <bjorn.topel@gmail.com> wrote:
>>>
>>> On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing@baidu.com> wrote:
>>>>
>>>> This fixes ice/i40e/ixgbe/ixgbevf_rx_buffer_flip in
>>>> copy mode xdp that can lead to data corruption.
>>>>
>>>> I split two patches, since i40e/xgbe/ixgbevf supports xsk
>>>> receiving from 4.18, put their fixes in a patch
>>>>
>>>
>>> Li, sorry for the looong latency. I took a looong vacation. :-P
>>>
>>> Thanks for taking a look at this, but I believe this is not a bug.
>>>
>>
>> Ok, dug a bit more into this. I had an offlist discussion with Li, and
>> there are two places (AFAIK) where Li experience a BUG() in
>> tcp_collapse():
>>
>>              BUG_ON(offset < 0);
>> and
>>                  if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>>                      BUG();
>>
>> (Li, please correct me if I'm wrong.)
>>
>> I still claim that the page-flipping mechanism is correct, but I found
>> some weirdness in the build_skb() call.
>>
>> In drivers/net/ethernet/intel/i40e/i40e_txrx.c, build_skb() is invoked as:
>>      skb = build_skb(xdp->data_hard_start, truesize);
>>
>> For the setup Li has truesize is 2048 (half a page), but the
>> rx_buf_len is 1536. In the driver a packet is layed out as:
>>
>> | padding 192 | packet data 1536 | skb shared info 320 |
>>
>> build_skb() assumes that the second argument (frag_size) is max packet
>> size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)). In other words,
>> frag_size should not include the padding (192 above). In build_skb(),
> 
> Not sure I am buying that reasoning. It assumes the padding + packet_data
> and we use skb_reserve() to tell the skb about the padding.
> 
> __build_skb_around() subtracts sizeof(struct skb_shared_info) from size
> that we are providing, so now we are with padding + packet_data.
> Then it is used to calculate the skb->end.
> 
> Back to i40e_build_skb(), we use the skb_reserve() to advance the
> skb->data and skb->tail so that they point to packet_data. Finally
> __skb_put() will move the skb->tail to the end of packet_data.
> 
> Wouldn't your approach disallow having the headroom at all in the linear
> part of skb?
>

Mea culpa.

You're perfectly right, and I'm all wrong. Thanks for sorting that out. 
xdp->data_hard_start messed up my neurons (if any one should ask).

*climbing back into the cave*


Sorry for the mail noise,
Björn