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

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?

[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;
	...
}

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

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

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

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

[ ... ]

[ ... ]

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