netdev - Re: [PATCH net v3] ipv4: allow local fragmentation in ip_finish_output

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, 3 Nov 2016 10:44:51 +0100
From:   Hannes Frederic Sowa <hannes@...essinduktion.org>
To:     Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Lance Richardson <lrichard@...hat.com>
Cc:     netdev@...r.kernel.org, fw@...len.de, jtluka@...hat.com
Subject: Re: [PATCH net v3] ipv4: allow local fragmentation in
 ip_finish_output_gso()

On 03.11.2016 08:42, Shmulik Ladkani wrote:
> On Wed,  2 Nov 2016 16:36:17 -0400
> Lance Richardson <lrichard@...hat.com> wrote:
> 
>> -	/* common case: fragmentation of segments is not allowed,
>> -	 * or seglen is <= mtu
>> +	/* common case: seglen is <= mtu
>>  	 */
>> -	if (((IPCB(skb)->flags & IPSKB_FRAG_SEGS) == 0) ||
>> -	      skb_gso_validate_mtu(skb, mtu))
>> +	if (skb_gso_validate_mtu(skb, mtu))
>>  		return ip_finish_output2(net, sk, skb);
> 
> OK.
> Resembles https://patchwork.ozlabs.org/patch/644724/ ;)
> 
>> -	if (skb_iif && !(df & htons(IP_DF))) {
>> -		/* Arrived from an ingress interface, got encapsulated, with
>> -		 * fragmentation of encapulating frames allowed.
>> -		 * If skb is gso, the resulting encapsulated network segments
>> -		 * may exceed dst mtu.
>> -		 * Allow IP Fragmentation of segments.
>> -		 */
>> -		IPCB(skb)->flags |= IPSKB_FRAG_SEGS;
>> -	}
> 
> The only potential issue I see removing this, is that the KNOWLEDGE of
> the forwarding/tunnel-bridging usecases (that require a
> skb_gso_validate_mtu check) is lost.
> 
> Meaning, if one decides (as claimed in the past) that locally generated
> traffic must NOT go through fragmentation validation, then no one
> remembers those other valid usecases (which are very specific and not
> trivial to imagine) that MUST go through it; Therefore it can easily get
> broken.

I agree but I think the git changelog reassembles the changes in this
area in good enough detail and it can be added if there is need for that
currently I don't see a reason why to leave this code there.

I am a bit sad to remove this condition, but the alternative, to
generate oversized frames, is absolutely not acceptable. :/

Thanks,
Hannes