netdev - Re: [QUESTION] Doubt about NAPI_GRO_CB(skb)->is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Dec 2017 17:16:03 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linuxarm@...wei.com" <linuxarm@...wei.com>,
        <yuxiaowu@...ilicon.com>, <wzhen.wang@...ilicon.com>,
        Xuehuahu <xuehuahu@...ilicon.com>
Subject: Re: [QUESTION] Doubt about NAPI_GRO_CB(skb)->is_atomic in tcpv4 gro
 process

Hi, Alexander

On 2017/12/21 0:24, Alexander Duyck wrote:
> On Wed, Dec 20, 2017 at 1:09 AM, Yunsheng Lin <linyunsheng@...wei.com> wrote:
>> Hi, all
>>         I have some doubt about NAPI_GRO_CB(skb)->is_atomic when
>> analyzing the tcpv4 gro process:
>>
>> Firstly we set NAPI_GRO_CB(skb)->is_atomic to 1 in dev_gro_receive:
>> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/core/dev.c#L4838
>>
>> And then in inet_gro_receive, we check the NAPI_GRO_CB(skb)->is_atomic
>> before setting NAPI_GRO_CB(skb)->is_atomic according to IP_DF bit in the ip header:
>> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/ipv4/af_inet.c#L1319
>>
>> struct sk_buff **inet_gro_receive(struct sk_buff **head, struct sk_buff *skb)
>> {
>> .....................
>>         for (p = *head; p; p = p->next) {
>> ........................
>>
>>                 /* If the previous IP ID value was based on an atomic
>>                  * datagram we can overwrite the value and ignore it.
>>                  */
>>                 if (NAPI_GRO_CB(skb)->is_atomic)                      //we check it here
>>                         NAPI_GRO_CB(p)->flush_id = flush_id;
>>                 else
>>                         NAPI_GRO_CB(p)->flush_id |= flush_id;
>>         }
>>
>>         NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF));  //we set it here
>>         NAPI_GRO_CB(skb)->flush |= flush;
>>         skb_set_network_header(skb, off);
>> ................................
>> }
>>
>> My question is whether we should check the NAPI_GRO_CB(skb)->is_atomic or NAPI_GRO_CB(p)->is_atomic?
>> If we should check NAPI_GRO_CB(skb)->is_atomic, then maybe it is unnecessary because it is alway true.
>> If we should check NAPI_GRO_CB(p)->is_atomic, maybe there is a bug here.
>>
>> So what is the logic here? I am just start analyzing the gro, maybe I miss something obvious here.
> 
> The logic there is to address the multiple IP header case where there
> are 2 or more IP headers due to things like VXLAN or GRE tunnels. So
> what will happen is that an outer IP header will end up being sent
> with DF not set and will clear the is_atomic value then we want to OR
> in the next header that is applied. It defaults to assignment on
> is_atomic because the first IP header will encounter flush_id with no
> previous configuration occupying it.

I see your point now.

But for the same flow of tunnels packet, the outer and inner ip header must
have the same fixed id or increment id?

For example, if we have a flow of tunnels packet which has fixed id in outer
header and increment id in inner header(the inner header does have DF flag set):

1. For the first packet, NAPI_GRO_CB(skb)->is_atomic will be set to zero when
inet_gro_receive is processing the inner ip header.

2. For the second packet, when inet_gro_receive is processing the outer ip header
which has a fixed id, NAPI_GRO_CB(p)->is_atomic is zero according to [1], so
NAPI_GRO_CB(p)->flush_id will be set to 0xFFFF, then the second packet will not
be merged to first packet in tcp_gro_receive.


I thought outer ip header could have a fixed id while inner ip header could
have a increment id. Do I miss something here?


> 
> The part I am not sure about is if we should be using assignment for
> is_atomic or using an "&=" to clear the bit and leave it cleared.

I am not sure I understood you here. is_atomic is a bit field, why do you
want to use "&="?


Thank very much for your time reqlying.
Yunsheng Lin

 I
> don't know if there has been much testing of multiple levels of tunnel
> header.
>> Thanks.
> 
> - Alex
> 
> .
>