Re: [PATCH v3 1/2] kho: add support for preserving vmalloc allocations

[Mike Rapoport <rppt@kernel.org>]

On Mon, Sep 08, 2025 at 08:12:59PM +0200, Pratyush Yadav wrote:
> On Mon, Sep 08 2025, Mike Rapoport wrote:
> 
> > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> >
> > A vmalloc allocation is preserved using binary structure similar to
> > global KHO memory tracker. It's a linked list of pages where each page
> > is an array of physical address of pages in vmalloc area.
> >
> > kho_preserve_vmalloc() hands out the physical address of the head page
> > to the caller. This address is used as the argument to
> > kho_vmalloc_restore() to restore the mapping in the vmalloc address
> > space and populate it with the preserved pages.
> >
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

...

> > @@ -742,6 +743,205 @@ int kho_preserve_phys(phys_addr_t phys, size_t size)
> >  }
> >  EXPORT_SYMBOL_GPL(kho_preserve_phys);
> >  
> > +struct kho_vmalloc_chunk;
> > +
> > +struct kho_vmalloc_hdr {
> > +	DECLARE_KHOSER_PTR(next, struct kho_vmalloc_chunk *);
> > +	unsigned int total_pages;	/* only valid in the first chunk */
> > +	unsigned int flags;		/* only valid in the first chunk */
> > +	unsigned short order;		/* only valid in the first chunk */
> > +	unsigned short num_elms;
> 
> I think it the serialization format would be cleaner if these were
> defined in a separate structure that holds the metadata instead of being
> defined in each page and then ignored in most of them.
> 
> If the caller can save 8 bytes (phys addr of first page), it might as
> well save 16 instead. Something like the below perhaps?
> 
> struct kho_vmalloc {
> 	DECLARE_KHOSER_PTR(first, struct kho_vmalloc_chunk *);
> 	unsigned int total_pages;
> 	unsigned short flags;
> 	unsigned short order;
> };
> 
> And then kho_vmalloc_hdr becomes simply:
> 
> struct kho_vmalloc_hdr {
> 	DECLARE_KHOSER_PTR(next, struct kho_vmalloc_chunk *);
> };
> 
> You don't even need num_elms since you have the list be zero-terminated.

Agree, thanks. 

> > +#define KHO_VMALLOC_FLAGS_MASK	(VM_ALLOC | VM_ALLOW_HUGE_VMAP)
> 
> I don't think it is a good idea to re-use VM flags. This can make adding
> more flags later down the line ugly. I think it would be better to
> define KHO_VMALLOC_FL* instead.

Ok.
 
> > +static void kho_vmalloc_free_chunks(struct kho_vmalloc_chunk *first_chunk)
> > +{
> > +	struct kho_mem_track *track = &kho_out.ser.track;
> > +	struct kho_vmalloc_chunk *chunk = first_chunk;
> > +
> > +	while (chunk) {
> > +		unsigned long pfn = PHYS_PFN(virt_to_phys(chunk));
> > +		struct kho_vmalloc_chunk *tmp = chunk;
> > +
> > +		__kho_unpreserve(track, pfn, pfn + 1);
> 
> This doesn't unpreserve the pages contained in the chunk, which
> kho_preserve_vmalloc() preserved.

Will fix. 

> > +	while (chunk) {
> > +		struct page *page;
> > +
> > +		for (int i = 0; i < chunk->hdr.num_elms; i++) {
> > +			phys_addr_t phys = chunk->phys[i];
> > +
> > +			for (int j = 0; j < (1 << order); j++) {
> > +				page = phys_to_page(phys);
> > +				kho_restore_page(page, 0);
> > +				pages[idx++] = page;
> 
> This can buffer-overflow if the previous kernel was buggy and added too
> many pages. Perhaps keep check for this?

You mean it added more than total_pages?
But the preserve part adds exactly vm->nr_pages, so once we get it right
what bugs do you expect here? 
 
> > +				phys += PAGE_SIZE;
> > +			}
> > +		}
> > +
> > +		page = virt_to_page(chunk);
> > +		chunk = KHOSER_LOAD_PTR(chunk->hdr.next);
> > +		kho_restore_page(page, 0);
> > +		__free_page(page);
> > +	}
> > +
> > +	area = __get_vm_area_node(nr * PAGE_SIZE, align, shift, flags,
> > +				  VMALLOC_START, VMALLOC_END, NUMA_NO_NODE,
> > +				  GFP_KERNEL, __builtin_return_address(0));
> > +	if (!area)
> > +		goto err_free_pages_array;
> > +
> > +	addr = (unsigned long)area->addr;
> > +	size = get_vm_area_size(area);
> > +	err = vmap_pages_range(addr, addr + size, PAGE_KERNEL, pages, shift);
> > +	if (err)
> > +		goto err_free_vm_area;
> > +
> > +	return area->addr;
> 
> You should free the pages array before returning here.

Why? They get into vm->pages.

-- 
Sincerely yours,
Mike.