linux-kernel - Re: [patch 4/6] perf/core: Split out ringbuffer 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: <87sehybcgo.ffs@tglx>
Date: Mon, 11 Aug 2025 08:26:47 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Linus Torvalds
 <torvalds@...uxfoundation.org>, Peter Zijlstra <peterz@...radead.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 4/6] perf/core: Split out ringbuffer allocation

On Thu, Aug 07 2025 at 16:38, Lorenzo Stoakes wrote:
> On Wed, Aug 06, 2025 at 10:12:58PM +0200, Thomas Gleixner wrote:
> THe second is that you don't seem to be doing:
>
> WARN_ON(!rb && event->rb);
> if (vma->vm_flags & VM_WRITE)
> 	flags |= RING_BUFFER_WRITABLE;
>
> In the aux code any more. Maybe first irrelevant, but second surely
> is?

Yeah. The first one is kinda silly. The second one I dropped unintentionally.

> DIFFERENCES:
>
> If we get to the rb_alloc_aux() bit, we're missing the:
>
> WARN_ON(!rb && event->rb);
> if (vma->vm_flags & VM_WRITE)
> 	flags |= RING_BUFFER_WRITABLE;
>
> Bit for aux case.
>
> Otherwise, it seems to be equivalent.

Thanks for taking the time to go through this.

>> -		if (rb_has_aux(rb)) {
>> -			atomic_inc(&rb->aux_mmap_count);
>> -			ret = 0;
>> -			goto unlock;
>> +		if (event->rb) {
>> +			ret = -EINVAL;
>
> Shouldn't this be if (!event->rb) ?
>
>> +		} else {
>
> Because here you're dereffing event->rb in branch where !event->rb?

Yes. I obviously failed to tested this particular patch alone and that's
fixed up in the next which moves the RB allocation out, so it did not
blow up in my face when I tested the whole pile.

Thanks for spotting!

       tglx