* [PATCH net-next] Revert "vxlan: move encapsulation warning"
@ 2020-09-26 1:56 Jakub Kicinski
2020-09-26 19:35 ` David Miller
2020-09-30 16:20 ` Fabian Frederick
0 siblings, 2 replies; 5+ messages in thread
From: Jakub Kicinski @ 2020-09-26 1:56 UTC (permalink / raw)
To: davem; +Cc: netdev, fabf, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"
2020-09-26 1:56 [PATCH net-next] Revert "vxlan: move encapsulation warning" Jakub Kicinski
@ 2020-09-26 19:35 ` David Miller
2020-09-30 16:20 ` Fabian Frederick
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2020-09-26 19:35 UTC (permalink / raw)
To: kuba; +Cc: netdev, fabf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"
2020-09-26 1:56 [PATCH net-next] Revert "vxlan: move encapsulation warning" Jakub Kicinski
2020-09-26 19:35 ` David Miller
@ 2020-09-30 16:20 ` Fabian Frederick
2020-09-30 16:29 ` Jakub Kicinski
1 sibling, 1 reply; 5+ messages in thread
From: Fabian Frederick @ 2020-09-30 16:20 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"
2020-09-30 16:20 ` Fabian Frederick
@ 2020-09-30 16:29 ` Jakub Kicinski
2020-10-01 16:51 ` Fabian Frederick
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2020-09-30 16:29 UTC (permalink / raw)
To: Fabian Frederick; +Cc: davem, netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] Revert "vxlan: move encapsulation warning"
2020-09-30 16:29 ` Jakub Kicinski
@ 2020-10-01 16:51 ` Fabian Frederick
0 siblings, 0 replies; 5+ messages in thread
From: Fabian Frederick @ 2020-10-01 16:51 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-01 16:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-26 1:56 [PATCH net-next] Revert "vxlan: move encapsulation warning" Jakub Kicinski
2020-09-26 19:35 ` David Miller
2020-09-30 16:20 ` Fabian Frederick
2020-09-30 16:29 ` Jakub Kicinski
2020-10-01 16:51 ` Fabian Frederick
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).