RE: [PATCH v2 2/2] hv_netvsc: Allocate host-visible GPADL buffers as decrypted contiguous chunks

["Kameron Carr" <kameroncarr@linux.microsoft.com>]

On Friday, July 31, 2026 8:47 AM, Michael Kelley wrote:
> From: Kameron Carr <kameroncarr@linux.microsoft.com> Sent: Thursday, July
30, 2026 4:34 PM
> >
> > On CoCo VMs without confidential VMBus, the netvsc send and receive
buffers
> > must be made host-visible by decrypting them. These buffers are
vmalloc'ed,
> > but set_memory_decrypted()/encrypted() do not work on vmalloc'ed memory.
> > This use case is (so far) unique to netvsc, so solve it locally rather
than
> > changing the set_memory() or allocation APIs.
> >
> > Add vmbus_alloc_buffer()/vmbus_free_buffer() to the VMBus core. When the
> > guest's isolation model requires it, allocate the buffer as a list of
> > physically-contiguous chunks via alloc_pages_node(), starting at
> > MAX_PAGE_ORDER and falling back to smaller orders so the allocation
still
> > succeeds under memory fragmentation. Each chunk is decrypted in place
via
> > set_memory_decrypted() on its direct-map address, and the chunks are
then
> > stitched into a single virtually-contiguous range with vmap(). Buffers
that
> > do not need decryption keep using vzalloc().
> >
> > This approach minimizes scattering of decrypted 4 KiB pages through the
> > kernel direct map and the resulting shattering of large page mappings.
> >
> > Use vmbus_establish_gpadl_caller_decrypted() so there is no attempt
> > to decrypt the virtual address. At teardown, vunmap() the range and
> > re-encrypt and free each chunk individually; any chunk that fails
> > re-encryption is leaked to prevent accidentally freeing decrypted
memory.
> >
> > Because vunmap() and set_memory_encrypted() must run in process context,
> > replace the rcu_head/call_rcu() pair used to defer free_netvsc_device()
> > with rcu_work/queue_rcu_work(). This also fixes a small race condition
> > where the buffers may be accessed while being re-encrypted by moving the
> > re-encryption after the RCU grace period.
> >
> > Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>
> > ---
> >  drivers/hv/channel.c            | 158 ++++++++++++++++++++++++++++++++
> >  drivers/net/hyperv/hyperv_net.h |   8 +-
> >  drivers/net/hyperv/netvsc.c     | 104 ++++++++++++++-------
> >  drivers/net/hyperv/netvsc_drv.c |   6 ++
> >  include/linux/hyperv.h          |   7 ++
> >  5 files changed, 249 insertions(+), 34 deletions(-)
> 
> Arguably, this patch should be broken into two patches. One
> patch adds the buffer allocation/free functions to the VMBus core.
> The second updates netvsc to use the new allocation/free
> functions and to call the free function in the required context.

Ack.

> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
[...]
> > +void *vmbus_alloc_buffer(struct vmbus_channel *channel,
> > +			 u32 size,
> > +			 struct page ***chunks_out,
> > +			 u32 *chunk_cnt_out)
> > +{
> > +	u32 nr_pages = PFN_UP(size);
> 
> Is there a reason for nr_pages to be u32, and then cast it to unsigned
long
> a couple of places below? The first argument to kvmalloc_array() is of
> type size_t.  Avoiding the casts would be incrementally cleaner.

Ack.

> > +	struct page **chunks = NULL;
> > +	struct page **pages = NULL;
> > +	unsigned int order;
> > +	u32 chunk_cnt = 0;
> > +	u32 page_idx = 0;
> > +	u32 remaining = nr_pages;
> > +	void *addr;
> > +	u32 i;
> > +	int ret;
> > +
> > +	*chunks_out = NULL;
> > +	*chunk_cnt_out = 0;
> > +
> > +	if (!nr_pages)
> > +		return NULL;
> > +
> > +	/* If the buffer does not need to be decrypted, just use vzalloc()
*/
> > +	if (!hv_is_isolation_supported() || channel->co_external_memory)
> > +		return vzalloc((unsigned long)nr_pages << PAGE_SHIFT);
> > +
> > +	/* Worst case: every chunk is a single page. */
> > +	chunks = kvmalloc_array(nr_pages, sizeof(*chunks),
> > +				GFP_KERNEL | __GFP_ZERO);
> > +	if (!chunks)
> > +		goto err;
> > +
> > +	pages = kvmalloc_array(nr_pages, sizeof(*pages), GFP_KERNEL);
> > +	if (!pages)
> > +		goto err;
> > +
> > +	/*
> > +	 * @order monotonically decreases across iterations
> > +	 *
> > +	 * Use __GFP_NORETRY | __GFP_NOWARN to avoid OOM-killing, but try
> > +	 * harder at order 0 since that is the final fallback.
> > +	 */
> > +	order = min(MAX_PAGE_ORDER, ilog2(nr_pages));
> 
> This works, but is more complex than needed. Just set order to
> MAX_PAGE_ORDER, and the min() at the top of the while loop below
> will do the right thing. And you could do order = MAX_PAGE_ORDER
> where order is declared.

Ack.

> > +	while (remaining) {
> > +		struct page *page;
> > +		gfp_t gfp;
> > +
> > +		order = min(order, ilog2(remaining));

Context: this fails to compile on x86 since order is unsigned and ilog2()
is signed.

Is it acceptable to avoid the min_t() by changing order to be a signed int?

> > +
> > +		for (;;) {
> > +			gfp = GFP_KERNEL | __GFP_ZERO;
> > +			if (order)
> > +				gfp |= __GFP_COMP | __GFP_NORETRY |
__GFP_NOWARN;
> > +			page =
alloc_pages_node(cpu_to_node(channel->target_cpu),
> > +						gfp, order);
> > +			if (page)
> > +				break;
> > +			if (!order)
> > +				goto err;
> > +			order--;
> > +		}
> 
> I think this nested for loop can be avoided. At this point in the outer
while
> loop, set the gfp flags and call alloc_pages_node() as you have here. The
> good and normal path is that alloc_pages_node() succeeds. The exception
> path is alloc_pages_node() failing, so you can do:
> 
> 		if (!page) {
> 			if (!order--)
> 				goto err;
> 			continue;
> 		}
> 
> The "continue" just restarts the outer while loop with the decremented
> value of "order" and everything proceeds normally. To me avoiding the
> nested loop is simpler, though "simpler" can be in the eye of the
beholder,
> so if you prefer to keep this as is, I'm OK with that.

I agree. Thank you for the suggestion.

> > +
> > +		ret = set_memory_decrypted((unsigned
long)page_address(page),
> > +					   1U << order);
> > +		if (ret) {
> > +			/*
> > +			 * set_memory_decrypted() failed; the page state is
> > +			 * unknown so it must be leaked rather than freed.
> > +			 */
> > +			goto err;
> > +		}
> > +
> > +		chunks[chunk_cnt++] = page;
> > +
> > +		for (i = 0; i < (1U << order); i++)
> > +			pages[page_idx++] = page + i;
> > +
> > +		remaining -= 1U << order;
> > +	}
> > +
> > +	addr = vmap(pages, nr_pages, VM_MAP, pgprot_decrypted(PAGE_KERNEL));
> > +	if (!addr)
> > +		goto err;
> > +
> > +	memset(addr, 0, (unsigned long)nr_pages << PAGE_SHIFT);
> > +
> > +	kvfree(pages);
> > +	*chunks_out = chunks;
> > +	*chunk_cnt_out = chunk_cnt;
> > +	return addr;
> > +
> > +err:
> > +	kvfree(pages);
> > +	vmbus_free_buffer(NULL, chunks, chunk_cnt);
> > +	return NULL;
> > +}
[...]
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 59e95341f9b1e..1192929d93a86 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -28,6 +28,8 @@
> >  #include "hyperv_net.h"
> >  #include "netvsc_trace.h"
> >
> > +static struct workqueue_struct *netvsc_wq;
> > +
> 
> Does netvsc needs its own workqueue to do the "free" operation,
> or would the system default workqueue (system_dfl_wq) be just
> as good? At first glance, the system_dfl_wq seems like it would work,
> since netvsc free operations are rare and don't have any strict
> latency requirements. But I'm far from being expect in workqueues,
> and there could be subtleties I'm not aware of.

I have to drain the work queue before module unload.
Currently this is handled by destroying the queue.

Originally, I tried using a system wide queue, but I saw that there are
compile time warnings, __warn_flushing_systemwide_wq(), which state:
"WARNING: Flushing system-wide workqueues will be prohibited in near
future."

I got the impression that having a driver specific work queue is best
practice.

- Kameron