Re: [PATCH v22 2/5] ring-buffer: Introducing ring-buffer mapping functions

David Hildenbrand
2024-05-10T09:19:27Z

On 08.05.24 04:34, Steven Rostedt wrote:
> On Tue, 30 Apr 2024 12:13:51 +0100
> Vincent Donnefort <vdonnefort@google.com> wrote:
>
>> +#ifdef CONFIG_MMU
>> +static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>> + struct vm_area_struct *vma)
>> +{
>> + unsigned long nr_subbufs, nr_pages, vma_pages, pgoff = vma->vm_pgoff;
>> + unsigned int subbuf_pages, subbuf_order;
>> + struct page **pages;
>> + int p = 0, s = 0;
>> + int err;
>> +
>> + /* Refuse MP_PRIVATE or writable mappings */
>> + if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
>> + !(vma->vm_flags & VM_MAYSHARE))
>> + return -EPERM;
>> +
>> + /*
>> + * Make sure the mapping cannot become writable later. Also tell the VM
>> + * to not touch these pages (VM_DONTCOPY | VM_DONTEXPAND). Finally,
>> + * prevent migration, GUP and dump (VM_IO).
>> + */
>> + vm_flags_mod(vma, VM_DONTCOPY | VM_DONTEXPAND | VM_IO, VM_MAYWRITE);
>
> Do we really need the VM_IO?
>
> When testing this in gdb, I would get:
>
> (gdb) p tmap->map->subbuf_size
> Cannot access memory at address 0x7ffff7fc2008
>
> It appears that you can't ptrace IO memory. When I removed that flag,
> gdb has no problem reading that memory.
>
> I think we should drop that flag.
>
> Can you send a v23 with that removed, Shuah's update, and also the
> change below:
>
>> +
>> + lockdep_assert_held(&cpu_buffer->mapping_lock);
>> +
>> + subbuf_order = cpu_buffer->buffer->subbuf_order;
>> + subbuf_pages = 1 << subbuf_order;
>> +
>> + nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
>> + nr_pages = ((nr_subbufs) << subbuf_order) - pgoff + 1; /* + meta-page */
>> +
>> + vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>> + if (!vma_pages || vma_pages > nr_pages)
>> + return -EINVAL;
>> +
>> + nr_pages = vma_pages;
>> +
>> + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
>> + if (!pages)
>> + return -ENOMEM;
>> +
>> + if (!pgoff) {
>> + pages[p++] = virt_to_page(cpu_buffer->meta_page);
>> +
>> + /*
>> + * TODO: Align sub-buffers on their size, once
>> + * vm_insert_pages() supports the zero-page.
>> + */
>> + } else {
>> + /* Skip the meta-page */
>> + pgoff--;
>> +
>> + if (pgoff % subbuf_pages) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>> +
>> + s += pgoff / subbuf_pages;
>> + }
>> +
>> + while (s < nr_subbufs && p < nr_pages) {
>> + struct page *page = virt_to_page(cpu_buffer->subbuf_ids[s]);
>> + int off = 0;
>> +
>> + for (; off < (1 << (subbuf_order)); off++, page++) {
>> + if (p >= nr_pages)
>> + break;
>> +
>> + pages[p++] = page;
>> + }
>> + s++;
>> + }
>
> The above can be made to:
>
> while (p < nr_pages) {
> struct page *page;
> int off = 0;
>
> if (WARN_ON_ONCE(s >= nr_subbufs))
> break;

I'm not particularly happy about us calling vm_insert_pages with NULL
pointers stored in pages.

Should we instead do

if (WARN_ON_ONCE(s >= nr_subbufs)) {
err = -EINVAL;
goto out;
}

?

--
Cheers,

David / dhildenb