netdev - Re: [net-next,v16,05/12] virtio_net: Query and set flow filter caps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b56861d-d940-4409-9a93-9f42cbc1b28f@nvidia.com>
Date: Thu, 29 Jan 2026 11:51:54 -0600
From: Dan Jurgens <danielj@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: mst@...hat.com, netdev@...r.kernel.org, pabeni@...hat.com,
 jasowang@...hat.com
Subject: Re: [net-next,v16,05/12] virtio_net: Query and set flow filter caps

On 1/22/26 7:43 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> virtio_net: Query and set flow filter caps
> 
> This patch adds flow filter capability support to virtio_net. It queries
> and sets flow filter capabilities during probe and restore, with cleanup
> during freeze and remove.
> 
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index db88dcaefb20..03bd3d2c2b0e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
> 
> [ ... ]
> 
>> @@ -5727,10 +5934,31 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>>  			return err;
>>  	}
>>
>> +	/* Initialize flow filters. Not supported is an acceptable and common
>> +	 * return code
>> +	 */
>> +	rtnl_lock();
>> +	err = virtnet_ff_init(&vi->ff, vi->vdev);
>> +	if (err && err != -EOPNOTSUPP) {
>> +		if (netif_running(vi->dev))
>> +			virtnet_close(vi->dev);
>> +
>> +		/* disable_rx_mmode_work takes the rtnl_lock, so just set the
>> +		 * flag here while holding the lock.
>> +		 *
>> +		 * remove_vq_common resets the device and frees the vqs.
>> +		 */
>> +		vi->rx_mode_work_enabled = false;
>> +		rtnl_unlock();
>> +		remove_vq_common(vi);
>> +		return err;
> 
> If virtnet_ff_init() fails here, remove_vq_common() frees vi->rq, vi->sq,
> and vi->ctrl via virtnet_free_queues(), but the netdevice remains
> registered. Could this leave the device in an inconsistent state where
> subsequent operations (like virtnet_open() triggered by bringing the
> interface up) would access freed memory through vi->rq[i]?
> 
> The error return propagates up to virtnet_restore() which just returns
> the error without further cleanup. If userspace then tries to use the
> still-registered netdevice, virtnet_open() would call try_fill_recv()
> which dereferences vi->rq.

It seems like there is a preexisting bug in this area.

Shouldn't we do virtnet_freeze_down and remove_vq_common on any error in
virtnet_restore_up?

The existing code doesn't suffer use-after-free, but the device is in a
bad state.

static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
{
	struct virtnet_info *vi = vdev->priv;
	int err;

	err = virtnet_restore_up(vdev);
	if (err)
		return err;
	virtnet_set_queues(vi, vi->curr_queue_pairs);

	err = virtnet_cpu_notif_add(vi);
	if (err) {
		virtnet_freeze_down(vdev);
		remove_vq_common(vi);
		return err;
	}

	return 0;
}

> 
>> +	}
>> +	rtnl_unlock();
>> +
>>  	netif_tx_lock_bh(vi->dev);
>>  	netif_device_attach(vi->dev);
>>  	netif_tx_unlock_bh(vi->dev);
>> -	return err;
>> +	return 0;
>>  }