[EXTERNAL] Re: [PATCH v2 0/2] net/netvsc: runtime queue reconfiguration and fixes

[Long Li]

> Subject: [EXTERNAL] Re: [PATCH v2 0/2] net/netvsc: runtime queue
> reconfiguration and fixes
> 
> On Wed, 25 Feb 2026 20:24:12 -0800
> Long Li <longli at microsoft.com> wrote:
> 
> > This series adds support for runtime queue count reconfiguration in
> > the netvsc PMD, along with a prerequisite bug fix.
> >
> > Patch 1 fixes a pre-existing subchannel resource leak in
> > eth_hn_dev_uninit() where subchannels are never closed before the
> > primary channel.
> >
> > Patch 2 adds runtime queue count reconfiguration via port
> > stop/configure/start, with full NVS/RNDIS session teardown and reinit
> > when the queue count changes.
> >
> > v2:
> > - Split subchannel leak fix into separate patch with Fixes tag (patch
> > 1)
> > - Fix reinit_failed recovery: re-map device before chan_open when device
> >   is unmapped to prevent undefined behavior on unmapped ring buffers
> > - Move hn_rndis_conf_offload() to after reinit block so offload config
> >   targets the final RNDIS session instead of being lost on teardown
> > - Use write lock in hn_vf_tx/rx_queue_release() to prevent race with
> >   concurrent fast-path readers holding read lock
> > - Reset RSS indirection table to queue 0 in subchan_cleanup error path
> > - Fix multi-line comment style to follow DPDK convention
> >
> > Long Li (2):
> >   net/netvsc: fix subchannel leak on device removal
> >   net/netvsc: support runtime queue count reconfiguration
> >
> >  drivers/net/netvsc/hn_ethdev.c | 181 +++++++++++++++++++++++++++++++--
> >  drivers/net/netvsc/hn_vf.c     |  16 +--
> >  2 files changed, 181 insertions(+), 16 deletions(-)
> >
> 
> After a couple of cycles with Claude, reduced the issues to more concise
> summary.
> Work is still needed.

I have sent v3 addressing those issues.

Thank you,
Long





> 
> ---
> 
> 
> 
> ## Patch 1: "fix subchannel leak on device removal"
> 
> Adds a loop in `eth_hn_dev_uninit` to close subchannels before closing the
> primary channel. This fixes a real resource leak I didn't explicitly call out
> (subchannels allocated in `hn_subchan_configure` were never closed on removal).
> 
> ## Patch 2: "support runtime queue count reconfiguration"
> 
> Major rework of `hn_dev_configure` to support changing queue counts at runtime,
> plus TX drain in `hn_dev_stop`, plus write-lock fix in VF queue release.
> 
> ---
> 
> ## Re-evaluation of my original findings
> 
> ### Finding #1: Double-free of `hv->primary` — STILL PRESENT
> 
> Neither patch changes this. The flow is still:
> 
> 1. `eth_hn_dev_uninit` → `hn_dev_close` → `hn_dev_free_queues` which calls
> `hn_rx_queue_free(rxq, false)` — this frees `hv->primary` via
> `hn_rx_queue_free_common` 2. `eth_hn_dev_uninit` then does `rte_free(hv-
> >primary)` at line 1448 — double-free
> 
> ### Finding #2: Init error path leaks `hv->primary` and `channels[0]` — STILL
> PRESENT
> 
> The `failed:` label in `eth_hn_dev_init` still only calls `hn_chim_uninit` and
> `hn_detach`. It never frees `hv->primary` or closes `hv->channels[0]`.
> 
> ### Finding #3: `hv->primary` alloc failure leaks `channels[0]` — STILL PRESENT
> 
> Lines 1376-1380 are untouched.
> 
> ### Finding #4: `max_chan <= 0` jumps to `failed` with `err` possibly zero — STILL
> PRESENT
> 
> Lines 1406-1407 are untouched. `err` is 0 from the successful
> `hn_rndis_get_eaddr` call, so the `failed:` path tears down the device and returns
> success.
> 
> ### Finding #5: `hn_dev_start` leaks event callback on rxfilter failure — STILL
> PRESENT
> 
> Lines 1035-1047 are untouched.
> 
> ---
> 
> ## New issues introduced by Patch 2
> 
> ### New Issue A: `hn_dev_configure` reinit path doesn't update `hv->primary-
> >chan`
> 
> When the reinit path closes the primary channel and opens a new one (lines in
> the patch around `rte_vmbus_chan_close(hv->channels[0])` ...
> `rte_vmbus_chan_open(hv->vmbus, &hv->channels[0])`), the `hv->primary` rx
> queue struct still holds a stale `chan` pointer from the old channel. `hv->primary-
> >chan` was set during `hn_rx_queue_alloc` at init time and is never updated here.
> The next control operation using `hv->primary` (which uses
> `hn_primary_chan(hv)` indirectly through `hn_nvs_execute` → `rxq->ring_lock`)
> would operate on the new channel correctly because `hn_primary_chan()` reads
> from `hv->channels[0]`, but `hv->primary->chan` is stale. This matters if anything
> uses `rxq->chan` directly for the primary queue.
> 
> ### New Issue B: `hn_chim_uninit`/`hn_chim_init` not called during reinit
> 
> The reinit path in `hn_dev_configure` calls `hn_detach` (which calls
> `hn_nvs_detach` → `hn_nvs_disconn_rxbuf` + `hn_nvs_disconn_chim`) and then
> `hn_attach` (which calls `hn_nvs_attach` → `hn_nvs_conn_chim`). After reinit, `hv-
> >chim_cnt` and `hv->chim_szmax` get updated by `hn_nvs_conn_chim`, but the
> chimney bitmap (`hv->chim_bmap`) still reflects the old allocation state. If
> `chim_cnt` changes across the reinit, the bitmap is the wrong size. This could lead
> to out-of-bounds bitmap access.
> 
> ### New Issue C: `reinit_failed` recovery doesn't restore `hv->primary-
> >rxbuf_info`
> 
> When `hn_attach` succeeds in the reinit path, `hn_nvs_conn_rxbuf` allocates a
> *new* `rxbuf_info` for `hv->primary`. But `hn_detach` (called earlier) doesn't free
> the old `rxbuf_info`. So the old one leaks, and the pointer is overwritten. In the
> recovery path, if `hn_attach` succeeds there too, another `rxbuf_info` is allocated,
> leaking the previous one.
> 
> ### New Issue D: VF queue release NULL-sets VF queues but VF setup doesn't re-
> set them
> 
> In `hn_vf_tx_queue_release`, the patch adds `vf_dev->data-
> >tx_queues[queue_id] = NULL`. This is good for preventing use-after-free.
> However, the corresponding `hn_vf_tx_queue_setup` calls
> `rte_eth_tx_queue_setup` on the VF which internally sets the VF's queue pointer.
> So this should be fine on re-setup — not a bug, just noting the asymmetry is
> intentional.
> 
> ---
> 
> ## Summary
> 
> All five original findings remain unfixed by these patches. The patches address
> different (legitimate) problems: subchannel leaks and runtime queue reconfig.
> The most critical outstanding issue is still the double-free of `hv->primary` (#1),
> and the new reinit path in Patch 2 introduces potential concerns around stale
> chimney bitmap state (#B) and leaked `rxbuf_info` (#C).