Re: [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization

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

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

- [High] u64_stats_sync write side in yt921x_mib_read() runs with
  preemption enabled.
- [Medium] Per-port MIB delayed work is permanently disabled after a port
  teardown/setup cycle.
- [Medium] The commit fixes a real data-race/tearing defect in already-
  merged driver code (the unsynchronized 64-bit MIB accessors and…
- [Low] The layout invariant tying yt921x_mib_descs[] to the u64_stats_t
  fields of struct yt921x_mib_stats is documented against the wrong…

Pre-existing issues:
- [Medium] In yt921x_dsa_get_rmon_stats() the ethtool RMON field
  `fragments` is filled from the alignment-error counter instead of the…
- [Medium] yt921x_chip_setup() unconditionally zeroes all hardware MIB
  counters (drivers/net/dsa/motorcomm/chip.c:4203, YT921X_MIB_CTRL_CLEAN…
- [Low] At the reviewed commit, the 64-bit MIB counters (RX_GOOD_BYTES
  0x3c/0x40, RX_BAD_BYTES 0x44/0x48, TX_GOOD_BYTES 0x84/0x88 — the…

--

net: dsa: motorcomm: Fix MIB synchronization

This converts the yt921x MIB accounting to u64_stats_sync for the
.get_stats64() path and extends priv->reg_lock coverage over the other
statistics callbacks.  A new per-port u64 data[] buffer collects register
reads so the counters can be published in one short write section.

(no Link: tags in the commit header)

net: dsa: motorcomm: Fix MIB synchronization

The previous MIB sync was based on the false assumption that 64-bit
variables are always atomic. To fix that:

[Severity: Medium]
Should this carry a Fixes: tag?  The unsynchronized 64-bit MIB accessors
and .get_stats64() came in with 186623f4aa724c ("net: dsa: yt921x: Add
support for Motorcomm YT921x"), so this is a fix to already-merged driver
code.

Related to that, the fix does not look self-contained: the counter type
conversion lives in the preceding patch "net: dsa: motorcomm: Use
u64_stats_t for MIB stats", whose changelog says "u64_stats_sync will be
equipped in the next commit", and this commit also builds on "Rename MIB
stuffs", "Split MIB buffers" and "Split MIB module".

Since neither commit alone closes the tearing window, how would a stable
backport of this fix be done without pulling in the whole refactor series?

[ ... ]

[Severity: Medium]
Does the MIB delayed work ever become schedulable again after a port
teardown?  yt921x_dsa_port_teardown() does:

	if (pm)
		disable_delayed_work_sync(&pm->work);

which increments the work item's disable count, and enable_work() documents
that "@work can only be queued if its disable count is 0".  There is no
enable_delayed_work() call anywhere in drivers/net/dsa/motorcomm/.

The initialization above (now including u64_stats_init()) sits inside the
"if (!pm && ...)" guard, and pm is devm_kzalloc()'d, so it survives a DSA
tree teardown/re-setup while the switch stays bound:

dsa_switch_remove() -> dsa_tree_teardown() -> yt921x_dsa_port_teardown()
dsa_register_switch() -> dsa_tree_setup() -> yt921x_dsa_port_setup()

On the second setup pp->mib is non-NULL, so the guarded block is skipped and
the disable count stays at 1.  Would schedule_delayed_work(&pm->work, 0) in
yt921x_phylink_mac_link_up() then silently do nothing, leaving the 32-bit
wraparound accumulation in yt921x_mib_read() stopped and .get_stats64()
returning frozen values?

The asymmetry was introduced by the earlier patch in this series, "net: dsa:
motorcomm: Split MIB buffers", which moved disable_delayed_work_sync() out
of yt921x_mdio_remove() into the new .port_teardown callback, and it is
still there at the series tip.

[Severity: Medium]
This isn't a bug introduced by this patch, but on the same teardown/re-setup
path, is the software shadow ever resynchronized with the hardware
counters?  yt921x_chip_setup() unconditionally clears them:

	/* Clear MIB */
	ctrl = YT921X_MIB_CTRL_CLEAN | YT921X_MIB_CTRL_ALL_PORT;
	res = yt921x_reg_write(priv, YT921X_MIB_CTRL, ctrl);

while pm (and now pm->data as well as pm->stats) is only zeroed at
devm_kzalloc() time above.  After the hardware counters go back to zero, the
wrap heuristic in yt921x_mib_read():

		u64 old_val = buf[i];

		val = (old_val & ~(u64)U32_MAX) | val0;
		if (val < old_val)
			val += 1ull << 32;

would see a small val0 against a large old_val and add 2^32, so every
32-bit counter would over-report by up to ~4.29e9 for the rest of the
driver's lifetime.  The comment "MIB buffer must be zeroed before" in mib.c
only holds for the first allocation.

[ ... ]

[Severity: Low]
This isn't a bug introduced by this patch, but the MIB_DESC(2, ...) counters
(RX_GOOD_BYTES 0x3c/0x40, RX_BAD_BYTES 0x44/0x48, TX_GOOD_BYTES 0x84/0x88)
are still composed from two independent SMI reads in the else branch just
below:

		} else {
			u32 val1;

			res = yt921x_reg_read(priv, reg + 4, &val1);
			if (res)
				break;
			val = ((u64)val1 << 32) | val0;
		}

If the counter crosses a 2^32 boundary between the two transactions, does
the stale low word get combined with the already incremented high word,
giving a ~4 GiB jump in rx_bytes/tx_bytes and Octets*OK?

The later patch in this series, "net: dsa: motorcomm: Use safe 64-bit
counter reader", replaces this with yt921x_counter_read(), which re-reads
the low word and retries until lo >= old_lo, so the concern does not survive
to the end of the series.

[Severity: High]
Should this write section run with preemption disabled?  yt921x_mib_read()
is only ever called with priv->reg_lock held, which is a sleepable mutex, and
all callers are fully preemptible process context:

yt921x_mib_poll()
    mutex_lock(&priv->reg_lock);
    yt921x_mib_read()
        u64_stats_update_begin(&pm->syncp);

include/linux/u64_stats_sync.h states as constraint (2) that the "Write side
must disable preemption, or a seqcount reader can preempt the writer and
also spin forever".

On BITS_PER_LONG==32, __u64_stats_update_begin() uses preempt_disable_nested(),
which does not disable preemption on non-PREEMPT_RT builds:

	if (IS_ENABLED(CONFIG_PREEMPT_RT))			\
		preempt_disable();				\
	else							\
		lockdep_assert_preemption_disabled();

So on 32-bit with CONFIG_PROVE_LOCKING, would this warn on every poll (every
YT921X_STATS_INTERVAL_JIFFIES, 3 s)?

And without lockdep, the sequence stays odd across the ~40 u64_stats_set()
stores plus the two histogram summations while preemption is enabled.  The
reader takes no mutex and can run with preemption off, for example:

bond_get_stats()
    spin_lock_nested(&bond->stats_lock)
    dev_get_stats() -> dsa_user_get_stats64() -> yt921x_dsa_get_stats64()
        u64_stats_fetch_begin(&pm->syncp)

If that reader lands on the same CPU as a preempted, CPU-bound kworker
writer, does it spin indefinitely in __read_seqcount_begin()?  Would
u64_stats_update_begin_irqsave()/u64_stats_update_end_irqrestore(), or an
explicit preempt_disable() around the write section, be the right form here?

[ ... ]

[Severity: Medium]
This is a pre-existing issue and not something this patch introduced, but a
few lines below in yt921x_dsa_get_rmon_stats() the RMON fragments field is
fed from the alignment error counter:

	rmon_stats->undersize_pkts = u64_stats_read(&mib->rx_undersize_errors);
	rmon_stats->oversize_pkts = u64_stats_read(&mib->rx_oversize_errors);
	rmon_stats->fragments = u64_stats_read(&mib->rx_alignment_errors);
	/* rmon_stats->jabbers */

struct ethtool_rmon_stats defines fragments as etherStatsFragments, and the
driver has a dedicated rx_fragment_errors counter (fed from
YT921X_MIB_DATA_RX_FRAG_ERR) which is only used in yt921x_dsa_get_stats64():

		stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) +
					  u64_stats_read(&mib->rx_fragment_errors);
		...
		stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors);

Should rmon_stats->fragments read rx_fragment_errors instead?

[ ... ]

[Severity: Low]
Could a static_assert() be added here to tie YT921X_MIB_NUM to
ARRAY_SIZE(yt921x_mib_descs)?  Both loops in yt921x_mib_read() are bounded
by ARRAY_SIZE(yt921x_mib_descs) while data[] is now sized from
sizeof(struct yt921x_mib_stats), and data[] is the last member of the
devm_kzalloc()'d struct yt921x_mib.

Both counts are 39 today, so nothing overruns at this commit, and no such
assertion appears later in the series.  Adding a descriptor without a
matching u64_stats_t field would turn what used to be an in-struct overrun
into a write past the end of the allocation.

Also, the invariant comment above yt921x_mib_descs[] in mib.c says:

/* Must agree with yt921x_mib
 *

Should that name yt921x_mib_stats, which is the struct YT921X_MIB_NUM
measures and the one the descriptor table actually indexes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com