netdev - Re: [net-next 2/3] ip_gre: fix erspan tunnel mtu calculation

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] [day] [month] [year] [list]

Date:   Wed, 11 Oct 2017 18:46:00 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     William Tu <u9012063@...il.com>
Cc:     davem <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [net-next 2/3] ip_gre: fix erspan tunnel mtu calculation

On Tue, Oct 10, 2017 at 8:59 PM, William Tu <u9012063@...il.com> wrote:
>>> @@ -1242,14 +1241,14 @@ static int erspan_tunnel_init(struct net_device *dev)
>>>         struct ip_tunnel *tunnel = netdev_priv(dev);
>>>         int t_hlen;
>>>
>>> -       tunnel->tun_hlen = 8;
>>> +       tunnel->tun_hlen = ERSPAN_GREHDR_LEN;
>>>         tunnel->parms.iph.protocol = IPPROTO_GRE;
>>>         tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
>>>                        sizeof(struct erspanhdr);
>>>         t_hlen = tunnel->hlen + sizeof(struct iphdr);
>>>
>>> -       dev->needed_headroom = LL_MAX_HEADER + t_hlen + 4;
>>> -       dev->mtu = ETH_DATA_LEN - t_hlen - 4;
>>> +       dev->needed_headroom = LL_MAX_HEADER + t_hlen;
>>> +       dev->mtu = ETH_DATA_LEN - t_hlen;
>> 1. I guess '+4-4' stuff was copied from __gre_tunnel_init(), I'm thinking
>> it may be there for some reason.
>>
> I traced back to
> 4565e9919cda ("gre: Setup and TX path for gre/UDP foo-over-udp encapsulation")
> and I think '+4-4' is there for GRE base header length.
>
> Since now we do
>     dev->mtu = ETH_DATA_LEN - t_hlen;
> and t_hlen already counts the the gre base header + optional header
> len, I think it's not needed.

okay. thanks.

>
>> 2. 'dev->needed_headroom =' and 'dev->mtu =' are really needed ?
>> As I've seen both will be updated in .newlink:
>> ipgre_newlink() -> ip_tunnel_newlink() -> ip_tunnel_bind_dev()
>>
> right, I also find both values gets overwritten by
> ip_tunnel_bind_dev() using my test cases. Maybe we can remove them?

It's there just in case that there is no lower dev found, but
ip_tunnel_newlink/create always updates dev->mtu even if
there is no lower dev found.

let's leave as it is for now, ipgre may just not be sure ip_tunnel_xxx would
do it when no lower dev found.