Re: [PATCH net-next 6/9] 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 allocated config with a
  plain store and immediately reclaims the old one with a bare kfree()…
- [Medium] The new configuration pointer is published with a plain store
  (vxlan_core.c:4038 `vxlan->cfg = new_cfg;`) right after the payload is…
- [Low] vxlan_encap_bypass() adds a `!dst_cfg` NULL test whose error
  branch (`goto drop`) lands on the `drop:` label, and that label passes…
- [Low] The patch introduces two mutually exclusive assumptions about
  vxlan->cfg for a live/registered netdev, on the same code paths.

--

vxlan: dynamically allocate struct vxlan_config

This moves struct vxlan_config out of struct vxlan_dev into a separately
allocated object referenced by vxlan->cfg, with vxlan_changelink() and
vxlan_dev_configure() allocating a new config and freeing the old one.
It is described as preparation for converting vxlan->cfg to RCU
protection in the following patch.

[Severity: High]
This per-packet RX lookup now walks other devices' cfg pointers with a
plain load.  Combined with the plain kfree() added in vxlan_config_apply()
below, can this dereference freed memory?

CPU0 (RTNL, ip link set ... type vxlan ...)
  vxlan_changelink()
    vxlan_config_apply()
      old_cfg = vxlan->cfg;
      vxlan->cfg = new_cfg;
      kfree(old_cfg);          <-- no grace period

CPU1 (RX softirq)
  vxlan_rcv()
    vxlan_vs_find_vni()
      cfg = node->vxlan->cfg;  <-- may already be old_cfg
      if (cfg->flags & VXLAN_F_VNIFILTER)   <-- use-after-free

[ ... ]

[Severity: Low]
Can cfg actually be NULL here?  The caller vxlan_stop() dereferences the
same pointer unconditionally a couple of statements earlier:

vxlan_stop()
  vxlan_multicast_leave(vxlan)
    if (vxlan->cfg->flags & VXLAN_F_VNIFILTER)   <-- no NULL check
  timer_delete_sync(&vxlan->age_timer);
  vxlan_flush(vxlan, &desc);
  vxlan_sock_release(vxlan)
    if (cfg && (cfg->flags & VXLAN_F_VNIFILTER)) <-- NULL check

The same disagreement shows up in several other pairs added by this patch:
vxlan_open() tests "cfg && cfg->age_interval" after vxlan_sock_add() and
__vxlan_sock_add() already read cfg->flags unguarded, vxlan_uninit() tests
"cfg &&" while its ndo_init counterpart vxlan_init() uses cfg->flags
directly, and vxlan_fdb_get() returns -ENODEV for a NULL cfg while
vxlan_fdb_parse() on the same registered device dereferences cfg->dst_port
and cfg->vni without a check.

Since vxlan->cfg is cleared only in vxlan_free_dev(), the priv_destructor,
which runs from free_netdev() after the device is unregistered (and
rtnl_newlink_create() calls free_netdev() directly on ->newlink failure
without invoking ndo_uninit), it looks like none of the netdev ops can
observe NULL, making these guards and the new -ENODEV path dead code.
Would it be clearer to drop the checks and keep the invariant uniform?
These mixed assumptions also remain at the end of the series.

[ ... ]

[Severity: Medium]
The pointer is loaded plainly here and used for many dereferences
(cfg->reserved_bits, cfg->flags) while vxlan_config_apply() publishes the
kmemdup()'ed replacement with a plain store.

Without a release barrier on the writer side (rcu_assign_pointer() or
smp_store_release()) and a dependency-ordering load on the reader side
(rcu_dereference()/READ_ONCE()), can a reader on a weakly ordered CPU see
the new pointer before the kmemdup'ed payload stores are visible, and act
on stale flags/reserved_bits/dst_port/saddr values?

The field is also declared without __rcu in include/net/vxlan.h, so sparse
cannot flag the missing accessors.  The following patch, "vxlan: convert
configuration to RCU protection", adds __rcu plus
rcu_assign_pointer()/rcu_dereference() at these sites, so would it be
reasonable to fold the annotation into this patch instead of leaving an
intermediate commit without ordering?

[ ... ]

[Severity: Low]
If the new !dst_cfg branch is ever taken, where does it land?  The goto
jumps to the drop label at the end of the same function, which passes the
same NULL dst_cfg straight into vxlan_vnifilter_count():

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

and this patch also removes the "!cfg ||" guard from that helper in
vxlan_vnifilter.c, so it now starts with:

	if (!(cfg->flags & VXLAN_F_VNIFILTER))
		return;

That would be a NULL dereference in softirq context.  The check appears to
be unreachable today, since vxlan->cfg is only cleared in vxlan_free_dev()
after the device is gone from the VNI hash, so is the new test needed at
all?  A later patch in the series, "vxlan: remove default_dst and use
vxlan_config and lowerdev", wraps the drop-path call in "if (dst_cfg)",
which suggests the intent was for the check to be live.

[ ... ]

[Severity: High]
This pointer is cached at function entry, outside any rcu_read_lock() in
this commit, and then handed to arp_reduce(), neigh_reduce(),
vxlan_xmit_one(), vxlan_xmit_nh(), vxlan_mdb_xmit() and
vxlan_vnifilter_count() for the duration of the transmit.  Can a
concurrent changelink free it underneath?

Before this patch the config was embedded in vxlan_dev and updated in
place, so a racing reader could only see a torn config.  Now the reader can
see freed slab memory, and vxlan_xmit_one() feeds cfg->saddr, cfg->ttl,
cfg->tos, cfg->label and cfg->dst_port into the encapsulation headers.

[ ... ]

[Severity: High]
The rcu_read_lock() here gives no protection against the plain kfree() in
vxlan_config_apply(), so can the timer softirq read cfg->age_interval from
a freed object across the whole FDB walk?

[ ... ]

[ ... ]

[ ... ]

[Severity: High]
This is the writer side of the use-after-free discussed above.  RTNL is
held here, but none of the readers (vxlan_xmit(), vxlan_rcv(),
vxlan_vs_find_vni(), vxlan_cleanup(), vxlan_fill_metadata_dst(),
mlxsw_sp_switchdev_vxlan_work_prepare()) hold RTNL, and there is no grace
period, refcount or shared lock between them.  Can the old config be freed
while an in-flight packet still dereferences it?

Since the commit message says this prepares for the RCU conversion in the
following patch, would it make sense to use kfree_rcu() (adding the
rcu_head in this patch) so the intermediate commit is not a bisect and
backport hazard?

[Severity: Medium]
Separately from the lifetime problem, this publishes a pointer to freshly
kmemdup()'ed data with a plain store.  Should this be rcu_assign_pointer()
(or at least smp_store_release()) so readers cannot observe the pointer
before the payload writes?

[ ... ]

[ ... ]

[Severity: Low]
Dropping the "!cfg ||" guard here makes every caller responsible for a
non-NULL cfg, which conflicts with the new !dst_cfg branch added in
vxlan_encap_bypass() that reaches its drop label with dst_cfg still NULL
and then calls this helper.  Should the two changes agree one way or the
other?

 
 	if (vninode) {

[ ... ]

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