Re: [PATCH v2 1/2] tracing: ring-buffer: Have the ring buffer code do the vmap of physical memory

[Kees Cook <kees@kernel.org>]

On Mon, Mar 31, 2025 at 01:39:06PM -0400, Steven Rostedt wrote:
> On Mon, 31 Mar 2025 09:55:28 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > Anyway, that takes care of the horrific interface. However, there's
> > another issue:
> > 
> > > +       pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);  
> > 
> > you create this pointless array of pages. Why? It's a physically
> > contiguous area.
> > 
> > You do that just because you want to use vmap() to map that contiguous
> > area one page at a time.
> > 
> > But this is NOT a new thing. It's exactly what every single PCI device
> > with a random physical memory region BAR needs to do. And no, they
> > don't create arrays of 'struct page *', because they use memory that
> > doesn't even have page backing.
> > 
> > So we actually have interfaces to do linear virtual mappings of
> > physical pages that *predate* vmap(), and do the right thing without
> > any of these games.
> 
> [ Added the pstore folks ]
> 
> OK, so I did copy this from fs/pstore/ram_core.c as this does basically the
> same thing as pstore. And it looks like pstore should be updated too.

I think we're talking about persistent_ram_vmap()? That code predates my
maintainership, but I'm happy to update it to use better APIs.

> > Yes, the legacy versions of interfaces are all for IO memory, but we
> > do have things like vmap_page_range() which should JustWork(tm).
> > 
> > Yeah, you'll need to do something like
> > 
> >         unsigned long vmap_start, vmap_end;
> > 
> >         area = get_vm_area(size, VM_IOREMAP);
> >         if (!area)
> >                 return NULL;
> > 
> >         vmap_start = (unsigned long) area->addr;
> >         vmap_end = vmap_start + size;
> > 
> >         ret = vmap_page_range(vmap_start, vmap_end,
> >                 *start, prot_nx(PAGE_KERNEL));
> > 
> >         if (ret < 0) {
> >                 free_vm_area(area);
> >                 return NULL;
> >         }
> > 
> > and the above is *entirely* untested and maybe there's something wrong
> > there, but the concept should work, and when you don't do it a page at
> > a time, you not only don't need the kmalloc_array(), it should even do
> > things like be able to use large page mappings if the alignment and
> > size work out.
> > 
> > That said, the old code is *really* broken to begin with. I don't
> > understand why you want to vmap() a contiguous physical range. Either
> > it's real pages to begin with, and you can just use "page_address()"
> > to get a virtual address, it's *not* real pages, and doing
> > "pfn_to_page()" is actively wrong, because it creates a fake 'struct
> > page *' pointer that isn't valid.
> > 
> > Is this all just for some disgusting HIGHMEM use (in which case you
> > need the virtual mapping because of HIGHMEM)? Is there any reason to
> > support HIGHMEM in this area at all?
> > 
> > So I'm not sure why this code does all this horror in the first place.
> > Either it's all just confused code that just didn't know what it was
> > doing and just happened to work (very possible..) or there is
> > something odd going on.

pstore tries to work with either real RAM or with iomem things. What
is there now Currently Works Fine, but should this be using
vmap_page_range()?

-- 
Kees Cook