From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6BEC4C43381 for ; Sun, 17 Mar 2019 23:52:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3CC8620854 for ; Sun, 17 Mar 2019 23:52:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727510AbfCQXwA (ORCPT ); Sun, 17 Mar 2019 19:52:00 -0400 Received: from laurent.telenet-ops.be ([195.130.137.89]:36590 "EHLO laurent.telenet-ops.be" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726333AbfCQXv7 (ORCPT ); Sun, 17 Mar 2019 19:51:59 -0400 Received: from [10.2.2.197] ([141.135.125.133]) by laurent.telenet-ops.be with bizsmtp id pBrw1z0062soW7901Brwil; Mon, 18 Mar 2019 00:51:57 +0100 Message-ID: <5C8EDDBB.8090900@mail.wizbit.be> Date: Mon, 18 Mar 2019 00:52:27 +0100 From: Bram Yvahk User-Agent: Thunderbird 2.0.0.24 (Windows/20100228) MIME-Version: 1.0 To: steffen.klassert@secunet.com, herbert@gondor.apana.org.au, davem@davemloft.net CC: netdev@vger.kernel.org Subject: Re: [PATCH ipsec/vti 1/2] vti: fragment IPv4 packets when DF bit is not set References: <1552865877-13401-1-git-send-email-bram-yvahk@mail.wizbit.be> <1552865877-13401-2-git-send-email-bram-yvahk@mail.wizbit.be> In-Reply-To: <1552865877-13401-2-git-send-email-bram-yvahk@mail.wizbit.be> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > --- > 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...]