Received-SPF: pass (google.com: domain of linux-kernel+bounces-175613-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 10 May 2024 11:57:27 +0100
From: Vincent Donnefort <vdonnefort@google.com>
To: David Hildenbrand <david@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>, mhiramat@kernel.org,
	linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
	mathieu.desnoyers@efficios.com, kernel-team@android.com,
	rdunlap@infradead.org, rppt@kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v22 2/5] ring-buffer: Introducing ring-buffer mapping
 functions
Message-ID: <Zj39l4EKmtNU6Rv6@google.com>
References: <20240430111354.637356-1-vdonnefort@google.com>
 <20240430111354.637356-3-vdonnefort@google.com>
 <20240507223402.206d6ddc@rorschach.local.home>
 <Zjyt-nX3el5nnDPm@google.com>
 <c36689df-cfad-46f5-835e-54073b6454a4@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c36689df-cfad-46f5-835e-54073b6454a4@redhat.com>

On Fri, May 10, 2024 at 11:15:59AM +0200, David Hildenbrand wrote:
> On 09.05.24 13:05, Vincent Donnefort wrote:
> > On Tue, May 07, 2024 at 10:34:02PM -0400, 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.
> > 
> > Yeah, VM_IO indeed implies DONTDUMP. VM_IO was part of Linus recommendations.
> 
> Yes, the VM should recognize that memory to some degree as being special
> already due to VM_MIXEDMAP and VM_DONTEXPAND.
> 
> #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP)
> 
> So any of these flag achieve that (e.g., mlock_fixup() checks VM_SPECIAL).
> KSM similarly skips VM_DONTEXPAND and VM_MIXEDMAP (likely we should be using
> VM_SPECIAL in vma_ksm_compatible()). Not sure about page migration, likely
> its fine.
> 
> Thinking about MADV_DONTNEED, I can spot in
> madvise_dontneed_free_valid_vma() only that we disallow primarily VM_PFNMAP.
> 
> ... I assume if user space MADV_DONTNEED's some pages we'll simply get a
> page fault later on access that will SIGBUS, handling that gracefully (we
> should double-check!).

I've just tested and indeed, I get a SIGBUS! All good there.

> 
> 
> > But perhaps, VM_DONTEXPAND and MIXEDMAP (implicitely set by vm_insert_pages) are
> > enough protection?
> 
> Do we want to dump these pages? VM_DONTDUMP might be reasonabe then. 

Somehow I thought this would prevent ptrace as well, but I've just tested it and
this is not the case as well. So let's keep DONTDUMP.

Thanks!

> 
> > 
> > I don't see how anything could use GUP there and as David pointed-out on the
> > previous version, it doesn't event prevent the GUP-fast path.
> 
> Yes, GUP-fast would still have worked under some conditions.
> 
> -- 
> Cheers,
> 
> David / dhildenb
>