Re: [net-next v1 4/6] net: stmmac: Add NCSI VLAN setting

[sashiko-bot@kernel.org]

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

New issues:
- [High] Bypassing STMMAC VLAN filter programming while enabling NETIF_F_HW_VLAN_CTAG_FILTER causes the host MAC to drop all VLAN-tagged packets.

Pre-existing issues:
- [High] Missing RCU grace period in NCSI VLAN kill path causes a Use-After-Free when traversing the VLAN list.
- [High] NCSI request timers can fire after the NCSI device is freed during unregistration.
--

commit f9348a76862879b362b9eac34abff037e5482a9e
Author: Minda Chen <minda.chen@starfivetech.com>

net: stmmac: Add NCSI VLAN setting

This commit adds NCSI VLAN setting support for STMMAC when the platform uses
NCSI. It modifies the driver to delegate VLAN addition and removal to the NCSI
subsystem and enables the hardware VLAN CTAG filter feature.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b6af537838830..b8ce04d71d08c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6866,6 +6866,9 @@ static int stmmac_vlan_rx_add_vid(struct net_device *ndev, __be16 proto, u16 vid
>  	bool is_double = false;
>  	int ret;
>  
> +	if (priv->plat->use_ncsi)
> +		return ncsi_vlan_rx_add_vid(ndev, proto, vid);
> +

[ ... ]

> @@ -6908,6 +6911,9 @@ static int stmmac_vlan_rx_kill_vid(struct net_device *ndev, __be16 proto, u16 vi
>  	bool is_double = false;
>  	int ret;
>  
> +	if (priv->plat->use_ncsi)
> +		return ncsi_vlan_rx_kill_vid(ndev, proto, vid);
> +

[Severity: High]
This isn't a bug introduced by this patch, but looking at the NCSI subsystem
this delegates to, does ncsi_vlan_rx_kill_vid() have an RCU grace period
issue?

net/ncsi/ncsi-manage.c:ncsi_vlan_rx_kill_vid() {
    ...
    list_del_rcu(&vlan->list);
    found = true;
    kfree(vlan);
    ...
}

Since concurrent NCSI workqueue tasks like set_one_vid() iterate over this
list using list_for_each_entry_rcu() inside an rcu_read_lock(), could failing
to wait for an RCU grace period before the kfree() lead to a use-after-free
when dynamic VLAN reconfiguration occurs?

[Severity: High]
This is a pre-existing issue, but since we are relying on NCSI, does the
NCSI core handle unregistration safely with respect to pending request timers?

net/ncsi/ncsi-manage.c:ncsi_unregister_dev() {
    ...
    disable_work_sync(&ndp->work);
    
    kfree(ndp);
}

If an NCSI command is pending during module removal or interface teardown, the
timer isn't canceled. Could ndp->requests[i].timer fire after ndp is freed,
causing a use-after-free in the ncsi_request_timeout() callback?

[ ... ]

> @@ -7943,6 +7949,9 @@ static int __stmmac_dvr_probe(struct device *device,
>  	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
>  			     NETDEV_XDP_ACT_XSK_ZEROCOPY;
>  
> +	if (priv->plat->use_ncsi)
> +		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +

[Severity: High]
Does enabling NETIF_F_HW_VLAN_CTAG_FILTER here cause the networking stack to
turn on the MAC's hardware VLAN filtering (VTFE)? 

Since stmmac_vlan_rx_add_vid() and stmmac_vlan_rx_kill_vid() were modified to
return early and bypass the STMMAC hardware filter programming, the filter
list will remain empty. Could this result in the MAC hardware aggressively
dropping all incoming VLAN-tagged packets, thereby breaking VLAN reception for
both the host OS and the NCSI subsystem?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610072420.64699-1-minda.chen@starfivetech.com?part=4