Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"

From: Fabian Frederick <fabf@skynet.be>
To: Jakub Kicinski <kuba@kernel.org>, davem@davemloft.net
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"
Date: Wed, 30 Sep 2020 18:20:05 +0200 (CEST)	[thread overview]
Message-ID: <121207656.52132.1601482805664@webmail.appsuite.proximus.be> (raw)
In-Reply-To: <20200926015604.3363358-1-kuba@kernel.org>

> On 26/09/2020 03:56 Jakub Kicinski <kuba@kernel.org> wrote:
> 
>  
> This reverts commit 546c044c9651e81a16833806feff6b369bb5de33.
> 
> Nothing prevents user from sending frames to "external" VxLAN devices.
> In fact kernel itself may generate icmp chatter.
> 
> This is fine, such frames should be dropped.
> 
> The point of the "missing encapsulation" warning was that
> frames with missing encap should not make it into vxlan_xmit_one().
> And vxlan_xmit() drops them cleanly, so let it just do that.
> 
> Without this revert the warning is triggered by the udp_tunnel_nic.sh
> test, but the minimal repro is:
> 
> $ ip link add vxlan0 type vxlan \
>      	      	     group 239.1.1.1 \
> 		     dev lo \
> 		     dstport 1234 \
> 		     external
> $ ip li set dev vxlan0 up
> 
> [  419.165981] vxlan0: Missing encapsulation instructions
> [  419.166551] WARNING: CPU: 0 PID: 1041 at drivers/net/vxlan.c:2889 vxlan_xmit+0x15c0/0x1fc0 [vxlan]
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/vxlan.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index fa21d62aa79c..be3bf233a809 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2651,6 +2651,11 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
>  		label = vxlan->cfg.label;
>  	} else {
> +		if (!info) {
> +			WARN_ONCE(1, "%s: Missing encapsulation instructions\n",
> +				  dev->name);
> +			goto drop;
> +		}
>  		remote_ip.sa.sa_family = ip_tunnel_info_af(info);
>  		if (remote_ip.sa.sa_family == AF_INET) {
>  			remote_ip.sin.sin_addr.s_addr = info->key.u.ipv4.dst;
> @@ -2885,10 +2890,6 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  		    info->mode & IP_TUNNEL_INFO_TX) {
>  			vni = tunnel_id_to_key32(info->key.tun_id);
>  		} else {
> -			if (!info)
> -				WARN_ONCE(1, "%s: Missing encapsulation instructions\n",
> -					  dev->name);
> -
>  			if (info && info->mode & IP_TUNNEL_INFO_TX)
>  				vxlan_xmit_one(skb, dev, vni, NULL, false);
>  			else
> -- 
> 2.26.2

Thanks a lot for explanations Jakub. udp_tunnel_nic.sh is a nice tool. Maybe it could also be used for remcsum testing ? I'd like to check net-next commit 2ae2904b5bac "vxlan: don't collect metadata if remote checksum is wrong" to make sure it has no impact as I had no ACK. Problem is ip encap-remcsum requires 'remote' specification not compatible with 'group' and only featuring in 'new_geneve' function in your script.

If both vxlan_parse_gbp_hdr() and vxlan_remcsum() require metadata recovery, I can reverse that patch and add some comment in vxlan_rcv()

Best regards,
Fabian