netdev - Re: [PATCH ipsec/vti 1/2] vti: fragment IPv4 packets when DF bit is not set

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: <5C8EDDBB.8090900@mail.wizbit.be>
Date:   Mon, 18 Mar 2019 00:52:27 +0100
From:   Bram Yvahk <bram-yvahk@...l.wizbit.be>
To:     steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        davem@...emloft.net
CC:     netdev@...r.kernel.org
Subject: Re: [PATCH ipsec/vti 1/2] vti: fragment IPv4 packets when DF bit
 is not set

Bram Yvahk wrote:
> Only send a 'need to frag' ICMP message when the "Don't Fragment" bit
> is set. If it's not set then the packet can/will be fragmented.
>
> This fixes sending an 'need to frag' message on a client that did not
> set the DF bit, i.e.:
>
>         $ ping -s 1300 -M dont -c5 192.168.235.2
>         PING 192.168.235.3 (192.168.235.3) 1300(1328) bytes of data.
>         From 192.168.236.254 icmp_seq=1 Frag needed and DF set (mtu = 1214)
>
> Signed-off-by: Bram Yvahk <bram-yvahk@...l.wizbit.be>
> ---
>  net/ipv4/ip_vti.c  | 43 +++++++++++++++++++++++++++--------------
>  net/ipv6/ip6_vti.c | 56 +++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 70 insertions(+), 29 deletions(-)
>
>   
Parts in the patch that I don't really like:

1) adds almost identical code in ipv4/ip_vti.c and ipv6/ip6_vti.c
    (is there a place to share this code?)

2) 'vti_tunnel_check_size'/'vti6_tunnel_check_size' functions:
   Name and implementation of this function is based on the xfrm code
   but to me "check" in the function name implies that it's (only)
   checking something but in this case it's doing a bit more: it also
updates
   the path-mtu and transmitting an ICMP message.

3) return value of 'vti6_tunnel_check_size':

   In XFRM ('xfrm4_tunnel_check_size'/'xfrm6_tunnel_check_size') return
   value of '0' and '-EMSGSIZE' is used.
  
   I opted not to follow this because of how the code is called in
   'vti6_xmit'. It has an 'err' variable but it is initialized to '-1'.
  
   What I considered:
   - let 'vti6_tunnel_check_size' return 0 and -EMSGSIZE: this matches
     with XFRM but it makes the 'err' variable in 'vti_xmit' a bit
confusing.
     Before calling 'vti6_tunnel_check_size' err=-1 means no error, after
     calling 'vti6_tunnel_check_size' err=0 means no error..
    
   - let 'vti6_tunnel_check_size' return 0 and -EMSGSIZE and use a second
     err variable in vti6_xmit (i.e.
       if ((err2 = vti6_tunnel_check_size(..)) < 0) { err=err2; goto ... }
  
   - let 'vti6_tunnel_check_size' return -1 and -EMSGSIZE: this matches
     with XFRM and works but to me it seems confusing for the function to
     return -1 when there is no error...
    
   - let 'vti6_tunnel_check_size' return a bool and set error in 'vti6_xmit'
  
   - change vti6_xmit to initialize err to 0 (but I've no idea of the impact
     of that)
    
   The least ugly solution to me was the bool so I went with that.
  
4) no error is set in vti_xmit (ip_vti): when the packet is too large it
   doesn't set an error and always returns 'NETDEV_TX_OK'. [This was already
   the case so that behaviour is kept but it doesn't look right...]