linux-kernel - Re: [patch V2 RESEND 4/6] perf/core: Split out AUX buffer allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <180cb0c2-0164-4c8d-947b-a86be2a2912e@lucifer.local>
Date: Tue, 12 Aug 2025 12:08:31 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>,
        Ingo Molnar <mingo@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Kees Cook <kees@...nel.org>
Subject: Re: [patch V2 RESEND 4/6] perf/core: Split out AUX buffer allocation

On Tue, Aug 12, 2025 at 12:06:13PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 11, 2025 at 02:21:08PM +0100, Lorenzo Stoakes wrote:
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Well, there's still that problem below. That was a goto unlock and would
> pass through the perf_mmap_account() and atomic_inc(mmap_count), where
> now it does not.

Yep, you're right, damn it.

I managed to miss this one among all the rest, and I was looking very very
carefully bit-by-bit... goes to show just how horrible the code was in teh first
place.

>
> Arguably it should not do the perf_mmap_account(), but lets keep the
> code functionally equivalent for now.
>
> Anyway, I've re-done these two break-out patches in a more fine grained
> fashion and will post a new series in a few.

OK will take a look.

>
> > > ---
> > > V2: Fixup invers condition and add the dropped flags setup back - Lorenzo
> > >     Fixup subject line to match the content
> > > ---
> > >  kernel/events/core.c |  137 +++++++++++++++++++++++++++++----------------------
> > >  1 file changed, 78 insertions(+), 59 deletions(-)
> > >
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -6970,12 +6970,79 @@ static void perf_mmap_account(struct vm_
> > >  	atomic64_add(extra, &vma->vm_mm->pinned_vm);
> > >  }
> > >
> > > +static int perf_mmap_aux(struct vm_area_struct *vma, struct perf_event *event,
> > > +			 unsigned long nr_pages)
> > > +{
> > > +	long user_extra = nr_pages, extra = 0;
> > > +	struct perf_buffer *rb = event->rb;
> > > +	u64 aux_offset, aux_size;
> > > +	int ret, rb_flags = 0;
> > > +
> > > +	/*
> > > +	 * AUX area mapping: if rb->aux_nr_pages != 0, it's already
> > > +	 * mapped, all subsequent mappings should have the same size
> > > +	 * and offset. Must be above the normal perf buffer.
> > > +	 */
> > > +	aux_offset = READ_ONCE(rb->user_page->aux_offset);
> > > +	aux_size = READ_ONCE(rb->user_page->aux_size);
> > > +
> > > +	if (aux_offset < perf_data_size(rb) + PAGE_SIZE)
> > > +		return -EINVAL;
> > > +
> > > +	if (aux_offset != vma->vm_pgoff << PAGE_SHIFT)
> > > +		return -EINVAL;
> > > +
> > > +	/* Already mapped with a different offset */
> > > +	if (rb_has_aux(rb) && rb->aux_pgoff != vma->vm_pgoff)
> > > +		return -EINVAL;
> > > +
> > > +	if (aux_size != nr_pages * PAGE_SIZE)
> > > +		return -EINVAL;
> > > +
> > > +	/* Already mapped with a different size */
> > > +	if (rb_has_aux(rb) && rb->aux_nr_pages != nr_pages)
> > > +		return -EINVAL;
> > > +
> > > +	if (!is_power_of_2(nr_pages))
> > > +		return -EINVAL;
> > > +
> > > +	/* If this succeeds, subsequent failures have to undo it */
> > > +	if (!atomic_inc_not_zero(&rb->mmap_count))
> > > +		return -EINVAL;
> > > +
> > > +	/* If mapped, attach to it */
> > > +	if (rb_has_aux(rb)) {
> > > +		atomic_inc(&rb->aux_mmap_count);
> > > +		return 0;
>
> here, the return 0 should've been a goto to...

Yeah... I think I had assumed we fixed this up somehow on the return but of
course we can't since we moved the perf_map_account() here (nor could we
differentiate return values).

But I think partly a product of the diff not showing that bit and, despite
checking line-by-line against old version I just about missed this one.

Yeah, this code, this refactor is very much needed!

>
> > > +	}
> > > +
> > > +	if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
> > > +		atomic_dec(&rb->mmap_count);
> > > +		return -EPERM;
> > > +	}
> > > +
> > > +	if (vma->vm_flags & VM_WRITE)
> > > +		rb_flags |= RING_BUFFER_WRITABLE;
> > > +
> > > +	ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
> > > +			   event->attr.aux_watermark, rb_flags);
> > > +	if (ret) {
> > > +		atomic_dec(&rb->mmap_count);
> > > +		return ret;
> > > +	}
> > > +
> > > +	atomic_set(&rb->aux_mmap_count, 1);
> > > +	rb->aux_mmap_locked = extra;
>
> here:
>
> > > +	perf_mmap_account(vma, user_extra, extra);
> > > +	atomic_inc(&event->mmap_count);
> > > +	return 0;
> > > +}