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

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

[Jakub Kicinski]

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

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

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 25 Sep 2020 18:56:04 -0700

> 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>

Applied, thanks Jakub.

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

[Fabian Frederick]

> 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

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

[Jakub Kicinski]

On Wed, 30 Sep 2020 18:20:05 +0200 (CEST) Fabian Frederick wrote:
> 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()

I think it's better if you create a separate script for that.

udp_tunnel_nic is supposed to be testing the NIC driver interface.

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

[Fabian Frederick]

> On 30/09/2020 18:29 Jakub Kicinski <kuba@kernel.org> wrote:
> 
>  
> On Wed, 30 Sep 2020 18:20:05 +0200 (CEST) Fabian Frederick wrote:
> > 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()
> 
> I think it's better if you create a separate script for that.
> 
> udp_tunnel_nic is supposed to be testing the NIC driver interface.

Looking at 'man ip link add', the only option to enable metadata seems 'external' which can't be declared with 'remote'

Result when trying to create vlxan:

vxlan: both 'external' and vni cannot be specified

Is there another way to check both VXLAN_F_COLLECT_METADATA and VXLAN_F_REMCSUM_RX ?

I just noticed that before commit f14ecebb3a4e 
("vxlan: clean up extension handling on rx")

checksum was tested before metadata collecting in vxlan_udp_encap_recv() so there should be no problem restoring initial behavior.

Best regards,
Fabian