Re: [PATCH net-next] vxlan: add IFLA_VXLAN_IGNORE_DF

Re: [PATCH net-next] vxlan: add IFLA_VXLAN_IGNORE_DF

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 1

- [High] vxlan_xmit_one() applies the new ignore_df knob to only half of
  the transmit decision in metadata (external / collect-metadata) mode.
- [Medium] IFLA_VXLAN_IGNORE_DF is declared as an unbounded `{ .type =
  NLA_U8 }` in vxlan_policy[] (drivers/net/vxlan/vxlan_core.c:3464), but…
- [Medium] Two statements in the commit message are not backed by the
  code.
- [Medium] IFLA_VXLAN_IGNORE_DF silently overrides IFLA_VXLAN_DF in the
  datapath — vxlan_xmit_one() evaluates `if (vxlan->cfg.ignore_df) { df…
- [Low] vxlan->cfg.ignore_df is re-read two (IPv4) or two (IPv6) separate
  times per transmitted packet in vxlan_xmit_one() without being…

--

vxlan: add IFLA_VXLAN_IGNORE_DF

Adds a new per-device netlink attribute that stops a vxlan device from
reporting a path MTU and dropping oversized frames, letting the outer IP
packet be fragmented instead. On IPv4 the DF bit is never set, on IPv6
skb->ignore_df is set so ip6_fragment() will fragment the encapsulated
frame.

A few questions below, plus two about the changelog itself.

> It also fails outright where the path forwards whole datagrams but
> discards IP fragments, which is common on carrier NAT. There the
> fragmenting has to happen inside the encapsulation to survive, and the
> tunnel refuses to do it.

[Severity: Medium]
Does the patch actually address this second scenario?  The mechanism added
here is outer-IP fragmentation only: df = 0 on IPv4 and skb->ignore_df = 1
on IPv6 so that ip6_fragment() proceeds.  Both produce IP fragments of the
encapsulating datagram, which a path that discards IP fragments drops the
same way it did before.  There is no inner-side fragmentation anywhere in
the change.

> Add IFLA_VXLAN_IGNORE_DF, off by default, mirroring IFLA_GRE_IGNORE_DF:
> carry the frame and let IP fragment the outer packet rather than report
> the path MTU and drop it.

Is "mirroring IFLA_GRE_IGNORE_DF" an accurate description of the scope?
In ip_tunnel_xmit(), GRE's ignore_df only suppresses inheriting the inner
DF bit, and the PMTU bookkeeping still runs right after:

net/ipv4/ip_tunnel.c:ip_tunnel_xmit() {
	if (payload_protocol == htons(ETH_P_IP) && !tunnel->ignore_df)
		df |= (inner_iph->frag_off & htons(IP_DF));

	if (tnl_update_pmtu(dev, skb, rt, df, inner_iph, 0, 0, false)) {
	...
}

This patch instead replaces the whole skb_tunnel_check_pmtu() call with a
constant 0, so neither skb_dst_update_pmtu_no_confirm() nor the
iptunnel_pmtud_check_icmp()/iptunnel_pmtud_check_icmpv6() reply used for
bridge and OVS ports runs any more.  Could the changelog mention that
difference?

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 459f19f7071e5..21cb7fbfb6e63 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2510,7 +2510,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			if (err)
>  				goto out_unlock;
>  
> -			if (vxlan->cfg.df == VXLAN_DF_SET) {
> +			if (vxlan->cfg.ignore_df) {
> +				df = 0;
> +			} else if (vxlan->cfg.df == VXLAN_DF_SET) {
>  				df = htons(IP_DF);
>  			} else if (vxlan->cfg.df == VXLAN_DF_INHERIT) {
>  				struct ethhdr *eth = eth_hdr(skb);

[Severity: Medium]
Should the combination of df and ignore_df be rejected instead of letting
one silently win?  With ignore_df set, cfg.df becomes dead configuration
here, but vxlan_nl2conf() accepts both without extack or -EINVAL and
vxlan_fill_info() keeps reporting nla_put_u8(skb, IFLA_VXLAN_DF,
vxlan->cfg.df).  So after

	ip link add vx0 type vxlan id 1 remote <ip> df set ignore_df 1

a dump says IFLA_VXLAN_DF = VXLAN_DF_SET while DF is never set on the wire.

GRE rejects the equivalent conflict:

net/ipv4/ip_gre.c:ipgre_netlink_parms() {
	if (data[IFLA_GRE_IGNORE_DF]) {
		if (nla_get_u8(data[IFLA_GRE_IGNORE_DF])
		  && (parms->iph.frag_off & htons(IP_DF)))
			return -EINVAL;
	...
}

> @@ -2526,7 +2528,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  		}
>  
>  		ndst = &rt->dst;
> -		err = skb_tunnel_check_pmtu(skb, ndst, vxlan_headroom(flags & VXLAN_F_GPE),
> +		err = vxlan->cfg.ignore_df ? 0 :
> +		      skb_tunnel_check_pmtu(skb, ndst,
> +					    vxlan_headroom(flags & VXLAN_F_GPE),
>  					    netif_is_any_bridge_port(dev));
>  		if (err < 0) {
>  			goto tx_error;

[Severity: High]
Can this drop packets silently in collect-metadata (external) mode?

The df computation above sits inside the "if (!info)" arm, while its
sibling arm in vxlan_xmit_one() still forces DF for metadata-driven
transmits:

drivers/net/vxlan/vxlan_core.c:vxlan_xmit_one() {
	} else if (test_bit(IP_TUNNEL_DONT_FRAGMENT_BIT,
			    info->key.tun_flags)) {
		df = htons(IP_DF);
	}
}

The new PMTU bypass, however, applies to both cases.  So for an
ignore_df=1 device in external mode carrying a tunnel key with
DONT_FRAGMENT set, the outer header has DF, the dst PMTU is not updated,
and no tunnel ICMP is sent back to the inner sender.  The packet then hits

net/ipv4/ip_output.c:ip_fragment() {
	if (unlikely(!skb->ignore_df ||
		     (IPCB(skb)->frag_max_size &&
		      IPCB(skb)->frag_max_size > mtu))) {
		IP_INC_STATS(net, IPSTATS_MIB_FRAGFAILS);
		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
			  htonl(mtu));
		kfree_skb(skb);
	...
}

and skb->ignore_df cannot help on IPv4 because iptunnel_xmit() calls
skb_scrub_packet(), which clears it.  Before this patch the same packet at
least got skb_dst_update_pmtu_no_confirm(), and iptunnel_pmtud_check_icmp()
for bridge/OVS ports.

The configuration is reachable because vxlan_nl2conf() accepts ignore_df
with no relation to VXLAN_F_COLLECT_METADATA, and OVS, act_tunnel_key,
nft_tunnel and lwtunnel can all set IP_TUNNEL_DONT_FRAGMENT_BIT.  Would it
make sense to keep the bypass in the same "if (!info)" scope as the df
computation, or to also clear DF on the metadata path?

[Severity: Low]
On a related note, vxlan->cfg.ignore_df is re-read here and again in the
df computation above without a local snapshot, while the changelink path
publishes a whole new config with

drivers/net/vxlan/vxlan_core.c:vxlan_config_apply() {
	memcpy(&vxlan->cfg, conf, sizeof(*conf));
}

under RTNL only, with no exclusion against the transmit path.  Would a
single "bool ignore_df = READ_ONCE(vxlan->cfg.ignore_df);" at the top of
vxlan_xmit_one() be tidier?  The other cfg fields on this path (cfg.df,
cfg.flags, cfg.ttl, cfg.tos, cfg.label_policy) are read the same way today,
so this is following existing convention rather than introducing something
new.

> @@ -2598,7 +2602,8 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  				goto out_unlock;
>  		}
>  
> -		err = skb_tunnel_check_pmtu(skb, ndst,
> +		err = vxlan->cfg.ignore_df ? 0 :
> +		      skb_tunnel_check_pmtu(skb, ndst,
>  					    vxlan_headroom((flags & VXLAN_F_GPE) | VXLAN_F_IPV6),
>  					    netif_is_any_bridge_port(dev));
>  		if (err < 0) {
> @@ -2630,6 +2635,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			goto tx_error;
>  		}
>  
> +		if (vxlan->cfg.ignore_df)
> +			skb->ignore_df = 1;
> +

[Severity: High]
Is the precedence relative to the tunnel key intended to differ between
the two address families?  This assignment is outside the "if (!info)"
scope, so on IPv6 an ignore_df device overrides an explicit per-packet
DONT_FRAGMENT request from the external controller, whereas on IPv4 the
tunnel key wins and DF stays set.

[Severity: Low]
These two IPv6 reads of vxlan->cfg.ignore_df are coupled: the PMTU check
is skipped based on the first one, and skb->ignore_df is set based on this
one.  If the flag flips from 1 to 0 in between, the frame gets neither PMTU
handling nor permission to fragment:

net/ipv6/ip6_output.c:ip6_fragment() {
	if (unlikely(!skb->ignore_df && skb->len > mtu))
		goto fail_toobig;
}

and it is dropped with no packet-too-big generated by the tunnel.

>  		udp_tunnel6_xmit_skb(ndst, sock6->sk, skb, dev,
>  				     &saddr, &pkey->u.ipv6.dst, tos, ttl,
>  				     pkey->label, src_port, dst_port, !udp_sum,
> @@ -3453,6 +3461,7 @@ static const struct nla_policy vxlan_policy[IFLA_VXLAN_MAX + 1] = {
>  	[IFLA_VXLAN_REMCSUM_NOPARTIAL]	= { .type = NLA_FLAG },
>  	[IFLA_VXLAN_TTL_INHERIT]	= { .type = NLA_FLAG },
>  	[IFLA_VXLAN_DF]		= { .type = NLA_U8 },
> +	[IFLA_VXLAN_IGNORE_DF]	= { .type = NLA_U8 },
>  	[IFLA_VXLAN_VNIFILTER]	= { .type = NLA_U8 },
>  	[IFLA_VXLAN_LOCALBYPASS]	= NLA_POLICY_MAX(NLA_U8, 1),
>  	[IFLA_VXLAN_LABEL_POLICY]       = NLA_POLICY_MAX(NLA_U32, VXLAN_LABEL_MAX),

[Severity: Medium]
Should this use NLA_POLICY_MAX(NLA_U8, 1) like the two neighbouring
boolean-style attributes in the same table (IFLA_VXLAN_LOCALBYPASS and
IFLA_VXLAN_MC_ROUTE)?  As written the full u8 range is accepted while the
value is stored in a bool, so "ignore_df 2" is taken, coerced to 1, and
read back as 1 by vxlan_fill_info().  Once released, the accepted range is
frozen and cannot be narrowed.

> @@ -4391,6 +4400,9 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
>  	if (data[IFLA_VXLAN_DF])
>  		conf->df = nla_get_u8(data[IFLA_VXLAN_DF]);
>  
> +	if (data[IFLA_VXLAN_IGNORE_DF])
> +		conf->ignore_df = nla_get_u8(data[IFLA_VXLAN_IGNORE_DF]);
> +

[Severity: Medium]
The GRE attribute this mirrors canonicalises the value in
ipgre_netlink_parms():

net/ipv4/ip_gre.c:ipgre_netlink_parms() {
	t->ignore_df = !!nla_get_u8(data[IFLA_GRE_IGNORE_DF]);
}

Would either a !! here or a range-limited policy entry be preferable to a
plain assignment into a bool?

[ ... ]

> @@ -4623,6 +4636,7 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		       !!(vxlan->cfg.flags & VXLAN_F_TTL_INHERIT)) ||
>  	    nla_put_u8(skb, IFLA_VXLAN_TOS, vxlan->cfg.tos) ||
>  	    nla_put_u8(skb, IFLA_VXLAN_DF, vxlan->cfg.df) ||
> +	    nla_put_u8(skb, IFLA_VXLAN_IGNORE_DF, vxlan->cfg.ignore_df) ||
>  	    nla_put_be32(skb, IFLA_VXLAN_LABEL, vxlan->cfg.label) ||
>  	    nla_put_u32(skb, IFLA_VXLAN_LABEL_POLICY, vxlan->cfg.label_policy) ||
>  	    nla_put_u8(skb, IFLA_VXLAN_LEARNING,
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 7b82075055237..85690b817e0ea 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -228,6 +228,7 @@ struct vxlan_config {
>  	unsigned int			addrmax;
>  	bool				no_share;
>  	enum ifla_vxlan_df		df;
> +	bool				ignore_df;
>  	struct vxlanhdr			reserved_bits;
>  };

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/c8add19a594ca2724ad97205c9cf3af387a2d6db.1788347880.git.daniel%40makrotopia.org

[PATCH net-next] vxlan: add IFLA_VXLAN_IGNORE_DF

[Daniel Golle]

A vxlan whose underlay cannot carry the encapsulated frame answers the
sender with a path MTU message and drops it. That is the right default,
but it leaves no way to extend a segment across an underlay that is
merely smaller: every host behind the tunnel then learns a path MTU that
the hosts beside it do not have, so once extended the resulting MTU can
be too small to carry any IPv6 at all (<1280).

It also fails outright where the path forwards whole datagrams but
discards IP fragments, which is common on carrier NAT. There the
fragmenting has to happen inside the encapsulation to survive, and the
tunnel refuses to do it.

Add IFLA_VXLAN_IGNORE_DF, off by default, mirroring IFLA_GRE_IGNORE_DF:
carry the frame and let IP fragment the outer packet rather than report
the path MTU and drop it.

On IPv4 this means never setting DF. On IPv6 there is no DF bit to
clear, and ip6_fragment() refuses a packet that is not from a local
socket unless skb->ignore_df is set, so set it there. Without that the
existing df attribute has no expression on IPv6 at all: an oversized
frame is dropped whatever it is set to.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/vxlan/vxlan_core.c | 20 +++++++++++++++++---
 include/net/vxlan.h            |  1 +
 include/uapi/linux/if_link.h   |  1 +
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 459f19f7071e..21cb7fbfb6e6 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2510,7 +2510,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			if (err)
 				goto out_unlock;
 
-			if (vxlan->cfg.df == VXLAN_DF_SET) {
+			if (vxlan->cfg.ignore_df) {
+				df = 0;
+			} else if (vxlan->cfg.df == VXLAN_DF_SET) {
 				df = htons(IP_DF);
 			} else if (vxlan->cfg.df == VXLAN_DF_INHERIT) {
 				struct ethhdr *eth = eth_hdr(skb);
@@ -2526,7 +2528,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		}
 
 		ndst = &rt->dst;
-		err = skb_tunnel_check_pmtu(skb, ndst, vxlan_headroom(flags & VXLAN_F_GPE),
+		err = vxlan->cfg.ignore_df ? 0 :
+		      skb_tunnel_check_pmtu(skb, ndst,
+					    vxlan_headroom(flags & VXLAN_F_GPE),
 					    netif_is_any_bridge_port(dev));
 		if (err < 0) {
 			goto tx_error;
@@ -2598,7 +2602,8 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 				goto out_unlock;
 		}
 
-		err = skb_tunnel_check_pmtu(skb, ndst,
+		err = vxlan->cfg.ignore_df ? 0 :
+		      skb_tunnel_check_pmtu(skb, ndst,
 					    vxlan_headroom((flags & VXLAN_F_GPE) | VXLAN_F_IPV6),
 					    netif_is_any_bridge_port(dev));
 		if (err < 0) {
@@ -2630,6 +2635,9 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			goto tx_error;
 		}
 
+		if (vxlan->cfg.ignore_df)
+			skb->ignore_df = 1;
+
 		udp_tunnel6_xmit_skb(ndst, sock6->sk, skb, dev,
 				     &saddr, &pkey->u.ipv6.dst, tos, ttl,
 				     pkey->label, src_port, dst_port, !udp_sum,
@@ -3453,6 +3461,7 @@ static const struct nla_policy vxlan_policy[IFLA_VXLAN_MAX + 1] = {
 	[IFLA_VXLAN_REMCSUM_NOPARTIAL]	= { .type = NLA_FLAG },
 	[IFLA_VXLAN_TTL_INHERIT]	= { .type = NLA_FLAG },
 	[IFLA_VXLAN_DF]		= { .type = NLA_U8 },
+	[IFLA_VXLAN_IGNORE_DF]	= { .type = NLA_U8 },
 	[IFLA_VXLAN_VNIFILTER]	= { .type = NLA_U8 },
 	[IFLA_VXLAN_LOCALBYPASS]	= NLA_POLICY_MAX(NLA_U8, 1),
 	[IFLA_VXLAN_LABEL_POLICY]       = NLA_POLICY_MAX(NLA_U32, VXLAN_LABEL_MAX),
@@ -4391,6 +4400,9 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
 	if (data[IFLA_VXLAN_DF])
 		conf->df = nla_get_u8(data[IFLA_VXLAN_DF]);
 
+	if (data[IFLA_VXLAN_IGNORE_DF])
+		conf->ignore_df = nla_get_u8(data[IFLA_VXLAN_IGNORE_DF]);
+
 	if (data[IFLA_VXLAN_VNIFILTER]) {
 		err = vxlan_nl2flag(conf, data, IFLA_VXLAN_VNIFILTER,
 				    VXLAN_F_VNIFILTER, changelink, false,
@@ -4547,6 +4559,7 @@ static size_t vxlan_get_size(const struct net_device *dev)
 		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_TTL_INHERIT */
 		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_TOS */
 		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_DF */
+		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_IGNORE_DF */
 		nla_total_size(sizeof(__be32)) + /* IFLA_VXLAN_LABEL */
 		nla_total_size(sizeof(__u32)) +  /* IFLA_VXLAN_LABEL_POLICY */
 		nla_total_size(sizeof(__u8)) +	/* IFLA_VXLAN_LEARNING */
@@ -4623,6 +4636,7 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		       !!(vxlan->cfg.flags & VXLAN_F_TTL_INHERIT)) ||
 	    nla_put_u8(skb, IFLA_VXLAN_TOS, vxlan->cfg.tos) ||
 	    nla_put_u8(skb, IFLA_VXLAN_DF, vxlan->cfg.df) ||
+	    nla_put_u8(skb, IFLA_VXLAN_IGNORE_DF, vxlan->cfg.ignore_df) ||
 	    nla_put_be32(skb, IFLA_VXLAN_LABEL, vxlan->cfg.label) ||
 	    nla_put_u32(skb, IFLA_VXLAN_LABEL_POLICY, vxlan->cfg.label_policy) ||
 	    nla_put_u8(skb, IFLA_VXLAN_LEARNING,
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 7b8207505523..85690b817e0e 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -228,6 +228,7 @@ struct vxlan_config {
 	unsigned int			addrmax;
 	bool				no_share;
 	enum ifla_vxlan_df		df;
+	bool				ignore_df;
 	struct vxlanhdr			reserved_bits;
 };
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 43cecca49f01..5a2d3a92941a 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1463,6 +1463,7 @@ enum {
 	IFLA_VXLAN_LABEL_POLICY, /* IPv6 flow label policy; ifla_vxlan_label_policy */
 	IFLA_VXLAN_RESERVED_BITS,
 	IFLA_VXLAN_MC_ROUTE,
+	IFLA_VXLAN_IGNORE_DF,
 	__IFLA_VXLAN_MAX
 };
 #define IFLA_VXLAN_MAX	(__IFLA_VXLAN_MAX - 1)
-- 
2.55.0