Re: [PATCH v2 net-next 5/8] vxlan: dynamically allocate struct vxlan_config

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

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

- [High] vxlan_config_apply() publishes the newly kmemdup()'ed
  configuration with a plain store and immediately reclaims the previous…
- [Medium] The freshly kmemdup()'ed vxlan_config is published with a
  plain assignment (`vxlan->cfg = new_cfg;`,…
- [Low] vxlan_encap_bypass() (drivers/net/vxlan/vxlan_core.c) newly
  treats `dst_vxlan->cfg == NULL` as a possible state (`if…
- [Low] vxlan_fdb_get() (drivers/net/vxlan/vxlan_core.c:1403-1421) adds
  NULL handling for vxlan->cfg only on the branch where NDA_VNI is…

--

vxlan: dynamically allocate struct vxlan_config

This moves struct vxlan_config out of struct vxlan_dev into a
dynamically allocated pointer vxlan->cfg.  vxlan_changelink() and
vxlan_dev_configure() now allocate a new config and free the old one.
A few questions below about the intermediate state this commit leaves
behind.

[ ... ]

[Severity: Low]
Are the two branches here meant to disagree about whether cfg can be
NULL?

When user space passes NDA_VNI the new -ENODEV check is skipped, and the
same possibly-NULL pointer is handed to vxlan_find_mac_rcu(), which
dereferences it right away:

	if (!(cfg->flags & VXLAN_F_COLLECT_METADATA))
		key.vni = cfg->vni;

Tracing it through, vxlan->cfg is set by vxlan_config_apply() from
vxlan_dev_configure() before register_netdevice(), and only cleared by
vxlan_free_dev() (the register_netdevice() failure path, or the
priv_destructor after unregistration), so a device reachable via
__dev_get_by_index() under RTNL always has a non-NULL cfg and the
-ENODEV branch looks like dead code.

Would it be clearer to drop the NULL test rather than have half of the
function defend against a state the other half assumes cannot happen?
The same asymmetry appears in vxlan_open() and vxlan_sock_release(),
where cfg is tested for NULL after vxlan_sock_add()/__vxlan_sock_add()
have already dereferenced it unconditionally.  It is still present at
the end of the series.

[ ... ]

[Severity: High]
Can this read freed memory?

vxlan_rcv() runs in softirq for every received frame and now caches a
plain pointer to a kmalloc()'ed object, while the only writer,
vxlan_config_apply(), releases the old object synchronously under RTNL:

	old_cfg = vxlan->cfg;
	vxlan->cfg = new_cfg;
	kfree(old_cfg);

So:

CPU0 (softirq)                     CPU1 (RTNL, ip link set ... type vxlan)
vxlan_rcv()
  cfg = vxlan->cfg;
                                   vxlan_changelink()
                                     vxlan_config_apply()
                                       vxlan->cfg = new_cfg;
                                       kfree(old_cfg);
  cfg->reserved_bits.vx_flags        <- freed

The same pattern applies to vxlan_vs_find_vni(), which dereferences
another device's cfg on every RX VNI lookup, and to vxlan_xmit(), which
propagates the cached pointer into arp_reduce(), neigh_reduce(),
vxlan_find_mac_tx(), vxlan_xmit_one() and vxlan_vnifilter_count().

The following patch in the series, "vxlan: convert configuration to RCU
protection", adds a struct rcu_head to struct vxlan_config, switches
publication to rcu_assign_pointer() plus kfree_rcu() and converts the
readers to rcu_dereference() inside RCU read sections, so the end state
of the series is fine.  Would it be better to fold that change in here,
or to at least use kfree_rcu() in this commit, so that this intermediate
commit is not a bisect or backport hazard on its own?

[ ... ]

[Severity: Low]
If the !dst_cfg case is really possible, does the drop label then
dereference NULL?

The goto lands on the shared drop label, which forwards the same pointer
on:

drop:
		dev_dstats_rx_dropped(dev);
		vxlan_vnifilter_count(dst_vxlan, dst_cfg, vni, NULL,
				      VXLAN_VNI_STATS_RX_DROPS, 0);

and this same commit removes the NULL tolerance from that helper (see
the vxlan_vnifilter_count() hunk below), so it starts with
cfg->flags.

As far as I can tell dst_vxlan->cfg cannot be NULL for a device that is
reachable through the VNI hash: devices are only inserted from
vxlan_vs_add_dev()/vxlan_vs_add_vnigrp() during ndo_open, and cfg is
populated before register_netdevice() and cleared only by
vxlan_free_dev().  So the new test appears unreachable, but caller and
callee end up with opposite contracts.

A later patch in the series, "vxlan: remove default_dst and use
vxlan_config and lowerdev", changes the drop label to guard the call
with if (dst_cfg).  Could that guard come with this hunk instead, or
could the !dst_cfg test be dropped here?

[ ... ]

[ ... ]

[Severity: High]
Does the rcu_read_lock() here buy anything while the writer uses plain
kfree()?

cfg is loaded once and read across the whole FDB walk in the ageing
timer softirq, but vxlan_config_apply() frees the old config
immediately, without waiting for a grace period, so this looks like the
same use-after-free window as in vxlan_rcv()/vxlan_xmit().

[ ... ]

[Severity: Medium]
Should the publication of new_cfg use release semantics?

The object comes straight out of kmemdup() and is published with a plain
store, while vxlan_rcv(), vxlan_xmit(), vxlan_vs_find_vni(),
vxlan_cleanup() and vxlan_fill_metadata_dst() consume it with plain
loads.  There is no rcu_assign_pointer()/smp_store_release() on the
write side and no rcu_dereference()/READ_ONCE() on the read side.

On weakly ordered architectures, can a softirq reader observe the new
pointer before the kmemdup() payload stores are visible, and then act on
stale cfg->flags, cfg->dst_port or cfg->reserved_bits?

The field is also declared as a plain pointer rather than __rcu (see the
include/net/vxlan.h hunk), so sparse cannot flag the missing
primitives.  The follow-up patch "vxlan: convert configuration to RCU
protection" adds the __rcu annotation and rcu_assign_pointer(), so this
gap exists only at this commit.

[Severity: Low]
Is removing the !cfg test here intentional in the same patch that adds a
NULL-tolerant caller?

vxlan_encap_bypass() now treats dst_vxlan->cfg == NULL as a reachable
state and still reaches this helper through its drop label, so the two
sides of the call disagree about the contract.

[Severity: Medium]
Since this pointer is already read locklessly from softirq contexts in
this commit, would annotating it __rcu here (as the following patch
does) make the missing rcu_dereference() sites visible to sparse right
away?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906180111.1973188-1-edumazet%40google.com