From: Bram Yvahk <bram-yvahk@mail.wizbit.be>
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
Date: Mon, 18 Mar 2019 00:52:27 +0100 [thread overview]
Message-ID: <5C8EDDBB.8090900@mail.wizbit.be> (raw)
In-Reply-To: <1552865877-13401-2-git-send-email-bram-yvahk@mail.wizbit.be>
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@mail.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...]
next prev parent reply other threads:[~2019-03-17 23:52 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-17 23:37 [PATCH ipsec/vti 0/2] Fragmentation of IPv4 in VTI Bram Yvahk
2019-03-17 23:37 ` [PATCH ipsec/vti 1/2] vti: fragment IPv4 packets when DF bit is not set Bram Yvahk
2019-03-17 23:52 ` Bram Yvahk [this message]
2019-03-17 23:37 ` [PATCH ipsec/vti 2/2] vti6: process icmp msg when IPv6 is fragmented Bram Yvahk
2019-03-21 15:16 ` [PATCH ipsec/vti 0/2] Fragmentation of IPv4 in VTI Steffen Klassert
2019-03-21 18:33 ` Bram Yvahk
2019-03-22 20:46 ` Bram Yvahk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5C8EDDBB.8090900@mail.wizbit.be \
--to=bram-yvahk@mail.wizbit.be \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=netdev@vger.kernel.org \
--cc=steffen.klassert@secunet.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).