Re: [PATCH 06/15] yunsilicon: add ethernet netdev registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] net: Negative error code truncated to u8 falsely reports Link UP on firmware failure
- [High] net: Out-of-bounds array write in XPS configuration when comp_vectors exceeds netdev queues
- [High] yunsilicon: TOCTOU race on link_event_handler causing NULL pointer dereference
- [High] net: carrier state desync due to lockless adapter->opened race
- [Medium] net: XPS cpumask accumulates CPUs across queue iterations breaking 1-to-1 mapping
- [Medium] net: Missing u64_stats_sync protection for 64-bit per-CPU statistics
- [Medium] net: xsc_eth_open silently ignores xsc_set_port_admin_status failure
- [Medium] net: Race between xsc_eth_close and link event handler leaves carrier ON while device closed
- [Medium] net: Race condition in xsc_eth_open silently drops valid hardware link events
- [Low] net: Hardware-provided MAC address is not validated before assignment
--

commit a6ac5cd27a115b0dca9b12956a0808733a45d432
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add ethernet netdev registration

This commit adds the ethernet auxiliary driver for Yunsilicon XSC devices. It allocates the netdev, programs the MAC address and MTU, registers it, and implements the basic open and stop operations for port admin status and link events.

[Severity: Medium]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_set_default_xps_cpumasks (around line 155), cpumask_clear is executed exactly once before the loop.

Inside the loop, cpumask_set_cpu adds a CPU for the current queue index. Because the shared mask is not cleared between iterations, CPUs accumulate across queues.

Could this break the intended Transmit Packet Steering isolation by mapping all traffic to the highest-indexed TX queue?

[Severity: Low]
Looking at drivers/net/ethernet/yunsilicon/xsc/net/main.c in xsc_eth_l2_addr_init (around line 318), the hardware-provided MAC address is retrieved via xsc_eth_get_mac.

If the hardware returns success, the retrieved MAC is directly applied to the netdevice without validation.

Should we check the address with is_valid_ether_addr to ensure the hardware doesn't supply an all-zero or multicast MAC address before setting it?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/pci/vport.c at xsc_core_query_vport_state (around line 28), the function returns a u8 status.

When xsc_cmd_exec fails, it returns a standard negative integer error code which is then truncated to a u8.

Back in drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_eth_get_link_status (around line 105), this truncated non-zero value is evaluated as a successful Link UP state.

Might this cause the driver to erroneously transition the carrier state to UP during a mailbox timeout or hardware failure?

[Severity: Medium]
In drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_stats.h (around line 11), struct xsc_rq_stats and struct xsc_sq_stats store 64-bit packet and byte counters as plain u64 fields.

Since these counters are updated in the NAPI softirq path and read globally without u64_stats_sync protection, will this result in torn reads on 32-bit platforms?

[Severity: Medium]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_eth_open (around line 189), the driver sends a hardware mailbox command via xsc_set_port_admin_status to physically enable the port.

The return value is discarded and the function unconditionally returns 0.

If the hardware initialization fails, could the kernel network stack incorrectly consider the interface UP while the physical port remains down?

[Severity: Medium]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_eth_close (around line 208), adapter->opened is set to false, followed by netif_carrier_off, and finally xsc_irq_event_unregister.

Is there a race condition where a concurrent link event handler passes the opened check before it flips, waits out the carrier status change, and then turns the carrier back ON just before the unregister step, leaving the carrier ON while the device is closed?

[Severity: Medium]
Also in drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_eth_open (around line 182), the event handler is registered before the adapter->opened flag is set to true.

If a hardware link state change triggers an interrupt in this exact window, wouldn't the async event handler execute, check the flag, and return early, thereby permanently dropping a valid hardware link event?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c at xsc_eth_build_nic_params (around line 74), the raw hardware capability is stored without capping it to the maximum channel size.

Then in xsc_set_default_xps_cpumasks (around line 155), the loop iterates up to this uncapped limit when calling netif_set_xps_queue.

If the hardware supports more completion vectors than allocated TX queues, could this trigger an out-of-bounds array write in the network core?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/pci/pci_irq.c at xsc_event_work (around line 309), the link_event_handler function pointer is checked and then called.

Concurrently, xsc_irq_event_unregister (around line 358) assigns NULL to the handler before flushing the workqueue.

Since READ_ONCE is not used, is it possible for the compiler to emit multiple memory loads, allowing the function pointer to become NULL between the check and the call, leading to a panic?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c, xsc_eth_link_event_handler (around line 138) performs a lockless read of adapter->opened.

Because this races with both ndo_open and ndo_stop, is there a risk that valid link UP events are dropped during open, or stale carrier ON events are applied after close, leading to a complete desync of the carrier state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=6