Re: [PATCH] xen-netback: wait for RCU readers before freeing queues on error

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 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 leaves the use-after-free open when the very first queue
  fails, which is every failure for a single-queue frontend.
- [Low] The commit message names xenvif_get_stats64() as one of the RCU
  readers, but xen-netback has no such function.

--

xen-netback: wait for RCU readers before freeing queues on error

This patch changes the connect() error path in xen-netback. It now
publishes num_queues = 0 and calls synchronize_net() before it deinits
and frees vif->queues, which is what backend_disconnect() already does.
The goal is to stop RCU stats readers from dereferencing the freed
vmalloc array.

> The net device is registered in xenvif_alloc() during probe, so it is
> already live when connect() runs. xenvif_get_stats64() and
> xenvif_get_ethtool_stats() read it through the usual stats paths, taking

[Severity: Low]
This isn't a bug, but xen-netback has no xenvif_get_stats64(). The driver
only implements the legacy callback:

drivers/net/xen-netback/interface.c:
	.ndo_get_stats	= xenvif_get_stats,

The KASAN trace also shows that function (xenvif_get_stats+0x28f). Should
the commit message say xenvif_get_stats() instead?

> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index e020f44ac2c71..d1c9e38cfb8ef 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -835,10 +835,21 @@ static void connect(struct backend_info *be)
>  	return;
>  
>  err:
> -	if (be->vif->num_queues > 0)
> +	if (be->vif->num_queues > 0) {
> +		unsigned int num_queues = be->vif->num_queues;
> +
>  		xenvif_disconnect_data(be->vif); /* Clean up existing queues */
> -	for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
> -		xenvif_deinit_queue(&be->vif->queues[queue_index]);
> +
> +		/*
> +		 * Mirror backend_disconnect(): publish num_queues == 0 and
> +		 * synchronize_net() so RCU stats readers finish before the free.
> +		 */
> +		be->vif->num_queues = 0;
> +		synchronize_net();
> +
> +		for (queue_index = 0; queue_index < num_queues; ++queue_index)
> +			xenvif_deinit_queue(&be->vif->queues[queue_index]);
> +	}
>  	vfree(be->vif->queues);

[Severity: High]
Is the use-after-free still open when the very first queue fails?

Earlier in connect(), num_queues is published before any queue is set up:

	be->vif->num_queues = requested_num_queues;
	be->vif->stalled_queues = requested_num_queues;

If xenvif_init_queue() or connect_data_rings() then fails for
queue_index 0, the failure branch resets the count. For example, when
connect_data_rings() fails:

	xenvif_deinit_queue(queue);
	be->vif->num_queues = queue_index;
	goto err;

This sets num_queues to 0, so the new if (be->vif->num_queues > 0) block
is skipped. synchronize_net() never runs, and vfree(be->vif->queues)
happens with no grace period.

A reader that sampled a non-zero num_queues during the
connect_data_rings() window could still be inside rcu_read_lock():

xenvif_get_stats() {
	rcu_read_lock();
	num_queues = READ_ONCE(vif->num_queues);
	for (index = 0; index < num_queues; ++index) {
		queue = &vif->queues[index];
	...
}

xenvif_get_ethtool_stats() follows the same pattern.

multi-queue-num-queues defaults to 1, and with one queue every failure
happens at queue_index 0. So for a default single-queue frontend, a guest
that supplies a bad ring reference or event channel would still reach
this vfree() without a grace period.

The new comment says this mirrors backend_disconnect(). However,
backend_disconnect() calls synchronize_net() before vfree(vif->queues)
whatever the queue count is.

The commit message says connect() jumps to err: "with num_queues still
counting the queues already set up". That doesn't hold in the queue 0
case.

Should the grace period run whenever vif->queues was allocated and
num_queues was published, and not only when num_queues > 0 at the err
label?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027121438.1266207-1-yhlee%40isslab.korea.ac.kr