Re: [PATCH] net/ncsi: use kfree_rcu_mightsleep for RCU-protected list entries

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 (2) · Medium: 0 · Low: 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] The fix depends on a premise the code does not support, so the
  use-after-free it targets is still reachable.
- [High] ncsi_unregister_dev() removes every package and channel (now via
  kfree_rcu_mightsleep) before it calls disable_work_sync(&ndp->work) at…
- [High] ncsi_remove_channel() still frees nc->mac_filter.addrs and
  nc->vlan_filter.vids right away with kfree() (ncsi-manage.c:253-254).
- [Low] net/ncsi: invalid commit SHA and subject in Fixes trailer

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  ncsi_unregister_dev() never cancels the per-request timers in…
- [High] This problem wasn't introduced by this patch, but
  ftgmac100_remove() calls ncsi_unregister_dev(priv->ndev)…

--

net/ncsi: use kfree_rcu_mightsleep for RCU-protected list entries

This commit changes kfree() to kfree_rcu_mightsleep() in
ncsi_remove_channel(), ncsi_remove_package() and ncsi_unregister_dev().
The channel, package and device structures are then freed only after an
RCU grace period, once list_del_rcu() has removed them from their lists.

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 1d63958c4429b..460fc024c1126 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -263,7 +263,7 @@ static void ncsi_remove_channel(struct ncsi_channel *nc)
>  	np->channel_num--;
>  	spin_unlock_irqrestore(&np->lock, flags);
>  
> -	kfree(nc);
> +	kfree_rcu_mightsleep(nc);

[Severity: Low]
The Fixes: tag doesn't look right. I can't find any commit that
2d283bdd5b20 resolves to. The subject "net/ncsi: Package and channel
management" belongs to a different commit, e6f44ed6d04d.

This commit introduced ncsi_remove_channel(), ncsi_remove_package()
and ncsi_unregister_dev(), along with the list_del_rcu() plus kfree()
sequences changed here:

  2d283bdd079c ("net/ncsi: Resource management")

The patch carries Cc: stable, so the stable tooling depends on this
trailer to pick the trees that need the backport. A SHA that doesn't
resolve, paired with the subject of another commit, will at best be
ignored and at worst point the backport at the wrong base.

Could you check which commit you meant and fix the trailer? Going by
the code being modified, it looks like it should be:

  Fixes: 2d283bdd079c ("net/ncsi: Resource management")

[Severity: High]
Does this grace period actually wait for any reader of these lists?

NCSI_FOR_EACH_DEV, NCSI_FOR_EACH_PACKAGE and NCSI_FOR_EACH_CHANNEL wrap
list_for_each_entry_rcu(). As far as I can tell, the only rcu_read_lock()
in net/ncsi/ is in set_one_vid(), and it covers ndp->vlan_vids. That is
why the VLAN fix mentioned in the commit message worked.

The readers of the package, channel and device lists run in process
context without an RCU read-side section. These include ncsi_find_dev(),
the genetlink handlers (ncsi_pkg_info_nl(), ncsi_pkg_info_all_nl(),
ncsi_set_interface_nl(), ncsi_clear_interface_nl(),
ncsi_set_channel_mask_nl()) and ndp->work helpers such as
ncsi_choose_active_channel() and ncsi_reset_dev().

For example, ndp_from_ifindex() returns ndp without holding a reference:

net/ncsi/ncsi-netlink.c:ndp_from_ifindex() {
    ...
	nd = ncsi_find_dev(dev);
	ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;

	dev_put(dev);
	return ndp;
}

ncsi_pkg_info_nl() then sleeps before it walks ndp->packages:

	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
	...
	rc = ncsi_write_package_info(skb, ndp, package_id);

NCSI_CMD_PKG_INFO does not need CAP_NET_ADMIN. ftgmac100_remove() calls
ncsi_unregister_dev() before unregister_netdev(), so dev_get_by_index()
and ncsi_find_dev() still succeed while packages and channels are being
removed.

The readers that do run under RCU are the packet_type RX handlers
(ncsi_rcv_rsp() and the AEN path). ncsi_unregister_dev() already drains
them with dev_remove_pack()->synchronize_net() before anything is freed,
and ncsi_remove_package() has no other caller.

So is the grace period only added where the RCU readers are already gone?
The concurrent accessors the commit message describes could then still
reach freed nc, np and ndp.

Since the patch carries Fixes: and Cc: stable, would a fix need
rcu_read_lock() on the reader side plus references for pointers used
after it? Another option would be a teardown that quiesces the netlink
and work users before freeing.

[Severity: High]
Should the channel's sub-allocations be deferred as well? Earlier in
ncsi_remove_channel(), while nc is still linked on np->channels, the
filter buffers are freed right away and the pointers are kept:

	spin_lock_irqsave(&nc->lock, flags);

	/* Release filters */
	kfree(nc->mac_filter.addrs);
	kfree(nc->vlan_filter.vids);

list_del_rcu(&nc->node) only runs after ncsi_stop_channel_monitor().
ncf->vids, ncf->n_vids and ncf->bitmap are never cleared.

Any reader that finds the channel can still follow these pointers, even
one holding rcu_read_lock(). NCSI_CMD_PKG_INFO is one such reader, and it
has .flags = 0 in ncsi_ops:

ncsi_pkg_info_nl()
  ncsi_write_package_info()
    ncsi_write_channel_info()

	while ((i = find_next_bit((void *)&ncf->bitmap, ncf->n_vids,
				  i + 1)) < ncf->n_vids) {
		if (ncf->vids[i])
			nla_put_u16(skb, NCSI_CHANNEL_ATTR_VLAN_ID,
				    ncf->vids[i]);
	}

Can this copy freed slab contents to an unprivileged netlink caller
during driver unbind?

set_one_vid() also reads ncf->vids under nc->lock. That lock gives no
protection once the remove path has dropped it.

>  }
>  
>  struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,

[ ... ]

> @@ -1969,6 +1969,6 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
>  
>  	disable_work_sync(&ndp->work);

[Severity: High]
Is ndp->work disabled too late? The removal loop in ncsi_unregister_dev()
runs before it:

	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
		ncsi_remove_package(np);
	...
	disable_work_sync(&ndp->work);

ncsi_dev_work() and the channel monitor timer reach channels through raw
pointers and ndp->channel_queue (nc->link), not inside RCU read-side
sections. The deferred free does not wait for them.

While the loop runs, ncsi_configure_channel() can still do this:

	ndp->hot_channel = hot_nc;
	...
	ncsi_start_channel_monitor(nc);

That re-arms nc->monitor.timer after ncsi_remove_channel() has stopped
it, so could ncsi_channel_monitor() later run on freed nc, np or ndp?

ncsi_process_next_channel() can also take a channel off
ndp->channel_queue and cache it:

	ndp->active_channel = nc;
	ndp->active_package = nc->package;

ncsi_remove_channel() only calls list_del_rcu(&nc->node). It does not
unlink nc->link from channel_queue. It also does not clear
ndp->active_channel, ndp->hot_channel or np->preferred_channel, so a
freed channel can stay reachable.

ncsi_stop_channel_monitor() also returns early, without
timer_delete_sync(), when monitor.enabled is already false:

	if (!nc->monitor.enabled) {
		spin_unlock_irqrestore(&nc->lock, flags);
		return;
	}

ncsi_channel_monitor() clears that flag itself on its timeout branch. It
then calls list_add_tail_rcu(&nc->link, &ndp->channel_queue) and
ncsi_process_next_channel().

Can a concurrent ncsi_remove_channel() skip waiting for that callback and
leave a dangling channel_queue entry?

Would it work better to quiesce the work and monitor timers, and unlink
nc->link, before removing the packages and channels?

>  
> -	kfree(ndp);
> +	kfree_rcu_mightsleep(ndp);

[Severity: High]
This isn't a bug introduced by this patch, but are the per-request timers
in ndp->requests[] ever shut down before ndp is freed?

ncsi_xmit_cmd() arms each request:

	nr->enabled = true;
	mod_timer(&nr->timer, jiffies + 1 * HZ);

Normally ncsi_rcv_rsp()->ncsi_free_request() cancels this timer. After
dev_remove_pack(), no response can arrive. A request still in flight at
unregister time keeps its timer pending, and its nr->cmd skb is never
released. Examples are a GLS sent by ncsi_channel_monitor() or an
NCSI_CMD_SEND_CMD request.

The kfree_rcu_mightsleep() grace period is unrelated to this 1 second
timer. When the timer fires, ncsi_request_timeout() takes ndp->lock and
may walk the freed package list through
ncsi_find_package_and_channel(). It then calls ncsi_free_request(), which
does schedule_work(&ndp->work) and consume_skb().

Can this leave a queued timer_list inside freed ndp memory? Nothing in
ncsi_unregister_dev() walks requests[] to call timer_shutdown_sync() or
ncsi_free_request().

[Severity: High]
This is also a pre-existing issue, but does ftgmac100 still use the NCSI
device after this free?

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_remove() {
    ...
	if (priv->ndev)
		ncsi_unregister_dev(priv->ndev);
	unregister_netdev(netdev);
    ...
}

priv->ndev and priv->use_ncsi are not cleared. If the interface is up,
unregister_netdev() closes it and ftgmac100_stop() calls:

	if (priv->use_ncsi)
		ncsi_stop_dev(priv->ndev);

ncsi_stop_dev() walks NCSI_FOR_EACH_PACKAGE(ndp, np). It then calls
ncsi_report_link(ndp, true), which calls nd->handler(nd). None of this
is inside an RCU read-side section.

Before this patch, this was an immediate use-after-free. With
kfree_rcu_mightsleep(), the grace period can still expire before the
device is closed, because unregister_netdev() takes rtnl and can block.

Doesn't this only change the timing window instead of fixing the bug?

>  }
>  EXPORT_SYMBOL_GPL(ncsi_unregister_dev);

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