[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 at gmail.com> wrote:
>>>
>>> On Fri, 17 Jul 2020 at 08:24, Li RongQing <lirongqing at 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