Re: [PATCH v8] net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In ftgmac100_remove(), the patch moves unregister_netdev() ahead
  of ncsi_unregister_dev().
- [High] In ncsi_aen_handler_lsc() (net/ncsi/ncsi-aen.c), the single-
  channel/single-package link-down path now sets `nc->state =…
- [High] The patch introduces an ABBA lock order inversion between
  ndp->lock and nc->lock.
- [Medium] After ncsi_stop_channel_monitor(), ncsi_aen_handler_lsc() and
  ncsi_aen_handler_cr() recheck only `nc->state == NCSI_CHANNEL_ACTIVE`…
- [Medium] Because of the reorder in ftgmac100_remove(), the NCSI device
  is still on ncsi_dev_list while unregister_netdev() runs.
- [Medium] The commit message describes only RCU-safe teardown,
  protection of Netlink/VLAN lookups, and synchronous monitor-timer stop.
- [Low] The new request-cleanup loop in ncsi_unregister_dev() calls
  ncsi_find_package_and_channel() for each outstanding NETLINK_DRIVEN…

Pre-existing issues:
- [Low] ncsi_send_netlink_timeout() falls back to reading the package ID
  from `((struct ncsi_pkt_hdr *)nr->cmd->data)->channel` when np is NULL.

--

net/ncsi: Fix Use-After-Free in NCSI teardown using synchronize_rcu

The patch unlinks the NCSI device from the global list and waits with
synchronize_rcu() before freeing packages and channels. It wraps the
Netlink lookups and VLAN callbacks in RCU/mutex protection and stops
channel monitors and request timers synchronously. It also reorders
ftgmac100_remove() so that unregister_netdev() runs before
ncsi_unregister_dev().

> NCSI device teardown can free packages and channels while RCU readers,
> including Netlink handlers, are still using them. Unlink the device from
> the global list and wait for RCU readers before freeing its state.
>
> Protect Netlink lookups and VLAN callbacks against concurrent teardown.
> Synchronously stop channel-monitor timers before freeing their channels,
> including when a timer callback has already cleared the enabled flag.
> Remove VLAN entries with RCU-safe list deletion and deferred freeing.

[Severity: Medium]
The commit message only covers the RCU teardown. The patch also seems to
include several independent changes, and these are described only in the
v7 -> v8 notes below the cut line, which are dropped when the patch is
applied:

- Six Netlink handlers no longer call get_net(sock_net(...)). There was
  no matching put_net() in net/ncsi/, so this fixes a struct net
  reference leak on every command.
- list_empty() guards against duplicate list_add_tail_rcu() on
  ndp->channel_queue in ncsi_channel_monitor(), the AEN handlers and
  ncsi_kick_channels().
- AEN state handling changes in ncsi_aen_handler_lsc() and
  ncsi_aen_handler_cr().
- vid is now initialised in set_one_vid().
- User-visible Netlink changes: the id > package_num - 1 check is removed
  from ncsi_write_package_info(), the PKG_INFO dump cursor now continues
  past package ID gaps, and genlmsg_cancel() is no longer called with a
  NULL hdr.
- Outstanding ncsi_request skbs and the vlan_vids entries are now freed
  at unregister.
- A new global ncsi_dev_mutex.

The message also says "RCU readers, including Netlink handlers". Before
this patch the Netlink handlers did not take rcu_read_lock().

Could these be split out, or at least described in the commit message,
with their own Fixes: tags where appropriate?

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 6d2fe5c2f3903..193f5147ef7a6 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -2107,9 +2109,11 @@ static void ftgmac100_remove(struct platform_device *pdev)
>  	netdev = platform_get_drvdata(pdev);
>  	priv = netdev_priv(netdev);
>
> -	if (priv->ndev)
> -		ncsi_unregister_dev(priv->ndev);
>  	unregister_netdev(netdev);
> +	if (priv->ndev) {
> +		ncsi_unregister_dev(priv->ndev);
> +		priv->ndev = NULL;
> +	}

[Severity: High]
Can this reorder trigger the BUG_ON in netdev_run_todo()?

ncsi_register_dev() binds the NCSI packet handler to this netdev:

    ndp->ptype.dev = dev;
    dev_add_pack(&ndp->ptype);

Because pt->dev is set, ptype_head() puts the handler on
dev->ptype_specific. The only removal is the dev_remove_pack() in
ncsi_unregister_dev(), and that now runs after unregister_netdev() has
returned.

unregister_netdev() -> rtnl_net_dev_unlock() -> rtnl_unlock() ->
netdev_run_todo() then checks:

    BUG_ON(!list_empty(&dev->ptype_specific));

dev_add_pack() takes no netdev reference, so netdev_wait_allrefs_any()
completes. The BUG_ON would then fire every time an NCSI-enabled
ftgmac100 is removed, whether by unbind or rmmod.

[Severity: Medium]
The NCSI device is now still on ncsi_dev_list while unregister_netdev()
runs. Does ncsi_vlan_rx_kill_vid() then restart the channel state machine
during teardown?

unregister_netdevice() first calls dev_close() -> ftgmac100_stop() ->
ncsi_stop_dev(). That function deliberately leaves the channel states
ACTIVE:

    /* Stop the channel monitor on any active channels. Don't reset the
     * channel state so we know which were active when ncsi_start_dev()
     * is next called.
     */

NETDEV_UNREGISTER handling in 8021q then removes the VLAN uppers and calls
vlan_kill_rx_filter_info() -> ncsi_vlan_rx_kill_vid(). That path checks
netif_device_present() but not whether the interface is up.
ncsi_find_dev() still succeeds, so ncsi_kick_channels() queues every
ACTIVE channel. ncsi_process_next_channel() -> ncsi_configure_channel()
then sends NC-SI commands, arms request timers and schedules ndp->work on
a closed netdev that is being unregistered.

Before this patch, ncsi_unregister_dev() ran first, so this callback
found no NCSI device and returned early.

[ ... ]

> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 040a31557201b..c4025e7f2cf26 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -93,8 +93,16 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
>  		if (had_link) {
>  			ndp->flags |= NCSI_DEV_RESHUFFLE;
>  			ncsi_stop_channel_monitor(nc);
> +			spin_lock_irqsave(&nc->lock, flags);
> +			if (nc->state != NCSI_CHANNEL_ACTIVE) {
> +				spin_unlock_irqrestore(&nc->lock, flags);
> +				return 0;
> +			}
> +			nc->state = NCSI_CHANNEL_INACTIVE;
> +			spin_unlock_irqrestore(&nc->lock, flags);
>  			spin_lock_irqsave(&ndp->lock, flags);
> -			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +			if (list_empty(&nc->link))
> +				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
>  			spin_unlock_irqrestore(&ndp->lock, flags);
>  			return ncsi_process_next_channel(ndp);
>  		}

[Severity: High]
Is setting NCSI_CHANNEL_INACTIVE here intended? Before this patch the
channel was queued with its state still ACTIVE, so
ncsi_process_next_channel() took the suspend path:

    case NCSI_CHANNEL_INACTIVE:
        ...
        ncsi_configure_channel(ndp);
        break;
    case NCSI_CHANNEL_ACTIVE:
        ...
        ncsi_suspend_channel(ndp);

With INACTIVE, the channel that has just lost link is reconfigured
instead of suspended. The skipped steps are SP, the GLS refresh on all
channels triggered by NCSI_DEV_RESHUFFLE, and DCNT/DC/DP. The failed
channel therefore stays enabled on the NIC, and its
modes[NCSI_MODE_TX_ENABLE].enable flag stays set.

When ncsi_choose_active_channel() then picks a replacement,
ncsi_channel_is_tx() sees the stale TX enable flag and returns false, so
the replacement never gets ECNT. If the replacement is in another
package and there is no HWA, the old package is never deselected either.

Does this break failover after a link loss in the default
!multi_package && !multi_channel configuration? It looks like the CR/kick
pattern may have been copied into the LSC path, and this change seems
unrelated to the teardown use-after-free.

> @@ -158,12 +166,15 @@ static int ncsi_aen_handler_cr(struct ncsi_dev_priv *ndp,
>
>  	ncsi_stop_channel_monitor(nc);
>  	spin_lock_irqsave(&nc->lock, flags);
> -	nc->state = NCSI_CHANNEL_INVISIBLE;
> +	if (nc->state != NCSI_CHANNEL_ACTIVE) {
> +		spin_unlock_irqrestore(&nc->lock, flags);
> +		return 0;
> +	}
> +	nc->state = NCSI_CHANNEL_INACTIVE;
>  	spin_unlock_irqrestore(&nc->lock, flags);
> -
>  	spin_lock_irqsave(&ndp->lock, flags);
> -	nc->state = NCSI_CHANNEL_INACTIVE;
> -	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +	if (list_empty(&nc->link))
> +		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
>  	spin_unlock_irqrestore(&ndp->lock, flags);

[Severity: Medium]
After ncsi_stop_channel_monitor(), both ncsi_aen_handler_cr() and
ncsi_aen_handler_lsc() recheck only nc->state, set INACTIVE and drop
nc->lock. Only after that do they take ndp->lock and test list_empty().

ncsi_kick_channels() does the check and the transition in a single
critical section:

    if (nc->state == NCSI_CHANNEL_ACTIVE &&
        list_empty(&nc->link)) {
        nc->state = NCSI_CHANNEL_INACTIVE;
        list_add_tail_rcu(&nc->link, &ndp->channel_queue);

The split sequence in the AEN handlers appears to allow two races.

First, timer_delete_sync() can wait for a monitor callback whose timeout
branch has already queued nc as ACTIVE. ncsi_channel_monitor() writes
nc->state under ndp->lock only. If another channel is ahead in the
queue, nc stays queued, and the AEN handler then overwrites its state
with INACTIVE. nc is later configured instead of suspended.

Second, a concurrent ncsi_reset_dev() -> ncsi_choose_active_channel()
(for example from a Netlink set_interface command) can run between the
nc->lock release and the ndp->lock acquisition. It can queue nc, and
ncsi_process_next_channel() can dequeue it and mark it INVISIBLE. The
AEN handler's list_empty() check then passes and it re-queues a channel
that is already being configured. The next dequeue hits the "Invalid
state" path, which calls ncsi_report_link(ndp, false).

Should these handlers check queue membership and change the state in the
same critical section as the enqueue?

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..266b1c4209ba4 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c

[ ... ]

> @@ -1567,13 +1567,12 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
>  		goto out;
>  	}
>
> +	spin_lock(&nc->lock);
>  	list_del_init(&nc->link);
> -	spin_unlock_irqrestore(&ndp->lock, flags);
> -
> -	spin_lock_irqsave(&nc->lock, flags);
>  	old_state = nc->state;
>  	nc->state = NCSI_CHANNEL_INVISIBLE;
> -	spin_unlock_irqrestore(&nc->lock, flags);
> +	spin_unlock(&nc->lock);
> +	spin_unlock_irqrestore(&ndp->lock, flags);

[Severity: High]
Does this create an ABBA ordering between ndp->lock and nc->lock?

Here and in ncsi_kick_channels(), nc->lock is now taken inside ndp->lock.
ncsi_choose_active_channel() is unchanged and still nests the locks the
other way round:

    spin_lock_irqsave(&nc->lock, cflags);
    ...
        spin_lock_irqsave(&ndp->lock, flags);
        list_add_tail_rcu(&nc->link,
                          &ndp->channel_queue);
        spin_unlock_irqrestore(&ndp->lock, flags);
    ...
    spin_unlock_irqrestore(&nc->lock, cflags);

For example:

CPU0
ncsi_vlan_rx_add_vid()
  ncsi_kick_channels()
    spin_lock_irqsave(&ndp->lock, flags);
    spin_lock(&nc->lock);          <- spins

CPU1
ncsi_choose_active_channel()
    spin_lock_irqsave(&nc->lock, cflags);
    spin_lock_irqsave(&ndp->lock, flags);   <- spins

CPU1 can be reached from ncsi_dev_work, from ncsi_reset_dev() via
Netlink, from an AEN in NET_RX softirq, or from the monitor timer.
ncsi_dev_mutex serializes only the VLAN, register and unregister paths.

Both CPUs have IRQs disabled, so this would be a hard lockup. Lockdep
should also see both orders during normal bring-up, even when no
deadlock occurs.

[ ... ]

> @@ -1956,19 +2003,56 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> +		struct ncsi_request *nr = &ndp->requests[i];
> +
> +		timer_delete_sync(&nr->timer);
> +		if (!nr->used)
> +			continue;
> +
> +		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN && nr->cmd) {
> +			struct ncsi_cmd_pkt *cmd = (struct ncsi_cmd_pkt *)
> +				skb_network_header(nr->cmd);
> +
> +			ncsi_find_package_and_channel(ndp,
> +						      cmd->cmd.common.channel,
> +						      &np, &nc);

[Severity: Low]
Will this produce an "RCU-list traversed in non-reader section!" warning
under CONFIG_PROVE_RCU_LIST?

ncsi_find_package_and_channel() goes through ncsi_find_package() and
ncsi_find_channel(). Those walk ndp->packages and np->channels with
NCSI_FOR_EACH_PACKAGE/NCSI_FOR_EACH_CHANNEL, which are
list_for_each_entry_rcu() without a lockdep condition. Here they are
called from process context with no rcu_read_lock().

There are no concurrent writers at this point, so this would only be a
debug splat. Elsewhere the patch does wrap similar helpers in
rcu_read_lock().

> +			ncsi_send_netlink_timeout(nr, np, nc);

[Severity: Low]
This isn't a bug introduced by this patch, but this new caller inherits
it. When np is NULL, ncsi_send_netlink_timeout() falls back to:

    nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
                NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)
                                     nr->cmd->data)->channel)));

ncsi_xmit_cmd() has already done skb_push(nr->cmd, sizeof(*eh)), so
nr->cmd->data points at the Ethernet header. Offset 5 is therefore
h_dest[5], which is 0xff for broadcast, and the reported package ID
becomes 7.

The teardown path reaches this fallback when an outstanding Netlink
command targets a package that was never discovered, because
ncsi_send_cmd_nl() only range-checks the package ID. The existing
ncsi_request_timeout() caller has the same problem.

> +		}
> +		ncsi_free_request(nr);
> +	}

[ ... ]

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