Re: [PATCH net] net: rionet: clear peer->res after releasing the outbound doorbell

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · 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 NULLs peer->res after the release in rionet_close() but
  does not fix the acquire/release mismatch that causes the bug.
- [Low] The commit message understates the bug it fixes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but rionet_close()
  clears nets[netid].active[destid] for every active peer…

--

net: rionet: clear peer->res after releasing the outbound doorbell

This patch sets peer->res to NULL after rionet_close() releases the
outbound doorbell resource. The goal is to stop rionet_remove_dev() from
releasing the same resource a second time.

> rionet_close() releases peer->res with rio_release_outb_dbell(), but
> leaves peer->res set. rionet_remove_dev() can releases peer->res again.
>
> Clear peer->res once it has been released.
>
> This bug was found statically and is theoretical.

[Severity: Low]
Is "theoretical" accurate here?

Before this patch, rionet_open() never requested peer->res again. A plain
ifup, ifdown, ifup, ifdown sequence made the second rionet_close() see the
stale pointer and call rio_release_outb_dbell() on it again:

drivers/rapidio/rio.c:rio_release_outb_dbell() {
	int rc = release_resource(res);

	kfree(res);
	...
}

That is a use-after-free in release_resource(), followed by a double
kfree().

An ifdown followed by rmmod rionet reaches the same release. rionet_exit()
calls subsys_interface_unregister() first, and that runs
rionet_remove_dev() for every peer.

The commit message only mentions the close->remove_dev path. Could it also
describe the close->close path, and say that the result is a
use-after-free and a double free? That would help with stable triage.

> diff --git a/drivers/net/rionet.c b/drivers/net/rionet.c
> index 64b5d0a8ae4a4..f8a7a778f2831 100644
> --- a/drivers/net/rionet.c
> +++ b/drivers/net/rionet.c
> @@ -387,8 +387,10 @@ static int rionet_close(struct net_device *ndev)
>  			rio_send_doorbell(peer->rdev, RIONET_DOORBELL_LEAVE);
>  			nets[netid].active[peer->rdev->destid] = NULL;
>  		}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Should
nets[netid].nact be decremented here too?

rionet_remove_dev() and the LEAVE branch of rionet_dbell_event() both
decrement nact when they clear the slot:

	nets[netid].active[rdev->destid] = NULL;
	nets[netid].nact--;

On the next rionet_open(), a JOIN doorbell goes out to every peer. Each
peer that answers reaches this code in rionet_dbell_event(), where
active[sid] is already NULL:

	if (!nets[netid].active[sid]) {
		...
			nets[netid].active[sid] = peer->rdev;
			nets[netid].nact++;

So every ifdown/ifup cycle increases nact by the number of active peers,
and it never comes back down.

rionet_start_xmit() uses nact as add_num for multicast and broadcast
frames:

	if (is_multicast_ether_addr(eth->h_dest))
		add_num = nets[rnet->mport->id].nact;

	if ((rnet->tx_cnt + add_num) > RIONET_TX_RING_SIZE) {
		netif_stop_queue(ndev);
		...
		return NETDEV_TX_BUSY;
	}

Once nact grows past RIONET_TX_RING_SIZE (128 by default), can any
broadcast such as ARP or IPv6 ND stop the queue even when tx_cnt is 0?

With nothing in flight, rionet_outb_msg_event() never runs to call
netif_wake_queue(). rionet_netdev_ops also has no ndo_tx_timeout. So TX
would seem to stay stalled until the next broadcast after every re-open.

rionet_remove_mport() also looks like it would trigger
WARN(nets[id].nact, ...) after any close done while peers were active.
rionet_remove_dev() only decrements nact for slots that are still active.

> -		if (peer->res)
> +		if (peer->res) {
>  			rio_release_outb_dbell(peer->rdev, peer->res);
> +			peer->res = NULL;
> +		}

[Severity: High]
Does this leave every peer without its outbound doorbell reservation after
the first ifdown?

peer->res is requested only once per peer, in rionet_add_dev():

	peer->res = rio_request_outb_dbell(peer->rdev,
					RIONET_DOORBELL_JOIN,
					RIONET_DOORBELL_LEAVE);

rionet_open() does not request it again. Its peer loop only sends JOIN:

	list_for_each_entry(peer, &nets[netid].peers, node) {
		/* Send a join message */
		rio_send_doorbell(peer->rdev, RIONET_DOORBELL_JOIN);
	}

After close and then open, the peer is still on nets[netid].peers and
keeps sending JOIN and LEAVE doorbells, but peer->res is NULL. The
JOIN..LEAVE range in rdev->riores[RIO_DOORBELL_RESOURCE] is free again,
so another request for it would succeed. rionet_remove_dev() then skips
the release without any warning.

rionet seems to be the only in-tree caller of rio_request_outb_dbell().
So nothing crashes today, but the resource ownership tracking is wrong.

Commit 34ed2ebb65b0 moved the request into rionet_add_dev() and added the
matching release in rionet_remove_dev(). It left the old release in
rionet_close().

Would it be more correct to remove the rio_release_outb_dbell() call from
rionet_close() entirely, so that rionet_remove_dev() does the only
release? That would also fix the double release, and peer->res would stay
held for as long as the peer exists.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104094811.19446-2-fourier.thomas%40gmail.com