netdev - Re: hard_header_len vs needed

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]

Message-ID: <CAHA+R7NERb0ZDbw7CX73A9VQSHs-+5DBfe=1-4rjPpoOGpnk8A@mail.gmail.com>
Date:	Mon, 9 Jun 2014 16:37:01 -0700
From:	Cong Wang <cwang@...pensource.com>
To:	Pravin Shelar <pshelar@...ira.com>
Cc:	David Miller <davem@...emloft.net>,
	Timo Teräs <timo.teras@....fi>,
	netdev <netdev@...r.kernel.org>
Subject: Re: hard_header_len vs needed_headroom

On Mon, Jun 9, 2014 at 11:16 AM, Pravin Shelar <pshelar@...ira.com> wrote:
> On Thu, Jun 5, 2014 at 4:39 PM, Cong Wang <cwang@...pensource.com> wrote:
>> Hello,
>>
>>
>> While debugging a skb header push crash, I noticed that
>> new ip tunnel code uses dev->needed_headroom as the
>> length of hardware header:
>>
>> dev->needed_headroom = t_hlen + hlen;
>>
>> in IPIP tunnel case it's the ethernet header length plus IPv4
>> header length. And it leaves dev->hard_header_len as 0.
>>
>> But there are some places assuming
>> dev->hard_header_len is the length of L2 header, at least
>> tcf_mirred() calls skb_push(skb, skb->dev->hard_header_len)
>> to reset skb->data back to L2 header. Current code is broken.
>>
>> I think ip tunnel code is wrong, according to LL_RESERVED_SPACE(),
>> it seems dev->hard_header_len should be the L2 header while
>> dev->needed_headroom should be the *additional* headroom needed
>> by hardware. That also means skb_push(dev->hard_header_len) is
>> correct.
>>
>> Therefore, I think IP tunnel should set its dev->hard_header_len to
>> lower_dev->hard_header_len + tunnel->hlen + sizeof(struct iphdr),
>> and set its dev->needed_headroom to lower_dev->needed_headroom.
>> See the attached patch for details.
>>
>> But gretap seems to be an exception, it calls ether_setup() to init its
>> dev->hard_header_len to ETH_HLEN. My patch probably breaks it,
>> I think it should set its tunnel->hlen = gre_hdr_len + ETH_HLEN.
>>
>
> When ip-tunnel was written I thought LL_RESERVED_SPACE() is used to
> reserve header space. Obviously I missed tcf_mirred()

It should be used to reserve headroom, however, mirred needs
->hard_header_len to reset skb->data back to mac header from network
header.

> At this point I think easiest fix would be using LL_RESERVED_SPACE()
> in tcf_mirred().
> Later we can change ip-tunnel code to use hard_header and
> needed_headroom as you have menioned.
>

It's not whether LL_RESERVED_SPACE() is used or not, it's whether
dev->hard_header_len is always the offset from mac_header to
network_header. mirred is not alone, tpacket_fill_skb() uses this as well.
So I think we have to fix dev->hard_header_len for tunnels.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html