linux-kernel - Re: [PATCH v3 06/15] perf: Move common code into both rb and aux branches

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9982e7b3-1046-4da1-9569-52bffe71c9e6@lucifer.local>
Date: Wed, 13 Aug 2025 06:55:51 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: tglx@...utronix.de, linux-kernel@...r.kernel.org,
        torvalds@...uxfoundation.org, mingo@...nel.org, namhyung@...nel.org,
        acme@...hat.com, kees@...nel.org
Subject: Re: [PATCH v3 06/15] perf: Move common code into both rb and aux
 branches

On Tue, Aug 12, 2025 at 12:39:04PM +0200, Peter Zijlstra wrote:
>   if (cond) {
>     A;
>   } else {
>     B;
>   }
>   C;
>
> into
>
>   if (cond) {
>     A;
>     C;
>   } else {
>     B;
>     C;
>   }
>

Hm we're doing more than that here though, we're refactoring other stuff at
the same time.

I guess you're speaking broad strokes here, but maybe worth mentioniing the
tricksy hobbitses around the rb_has_aux() bit.

Anyway LGTM so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  kernel/events/core.c |   25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7043,6 +7043,8 @@ static int perf_mmap(struct file *file,
>  				ret = 0;
>  				/* We need the rb to map pages. */
>  				rb = event->rb;
> +				perf_mmap_account(vma, user_extra, extra);
> +				atomic_inc(&event->mmap_count);
>  				goto unlock;
>  			}
>
> @@ -7083,6 +7085,9 @@ static int perf_mmap(struct file *file,
>  		perf_event_init_userpage(event);
>  		perf_event_update_userpage(event);
>  		ret = 0;
> +
> +		perf_mmap_account(vma, user_extra, extra);
> +		atomic_inc(&event->mmap_count);
>  	} else {
>  		/*
>  		 * AUX area mapping: if rb->aux_nr_pages != 0, it's already
> @@ -7127,11 +7132,12 @@ static int perf_mmap(struct file *file,
>  		if (rb_has_aux(rb)) {
>  			atomic_inc(&rb->aux_mmap_count);
>  			ret = 0;
> -			goto unlock;
> +			goto aux_success;
>  		}
>
>  		if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
>  			ret = -EPERM;
> +			atomic_dec(&rb->mmap_count);
>  			goto unlock;
>  		}
>
> @@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file,
>
>  		ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
>  				   event->attr.aux_watermark, flags);
> -		if (!ret) {
> -			atomic_set(&rb->aux_mmap_count, 1);
> -			rb->aux_mmap_locked = extra;
> +		if (ret) {

You dropped the 'AUX allocation failed' comment here, but honestly I think
that's fine, it's pretty obvious that's the case given the literal previous
line is you trying the AUX allocation... :)

> +			atomic_dec(&rb->mmap_count);
> +			goto unlock;
>  		}
> -	}
>
> -unlock:
> -	if (!ret) {
> +		atomic_set(&rb->aux_mmap_count, 1);
> +		rb->aux_mmap_locked = extra;
> +aux_success:
>  		perf_mmap_account(vma, user_extra, extra);
>  		atomic_inc(&event->mmap_count);
> -	} else if (rb) {
> -		/* AUX allocation failed */
> -		atomic_dec(&rb->mmap_count);
>  	}
> +
> +unlock:
>  aux_unlock:

Hm, this sucks, I assume this is a temporary thing to reduce churn above
and effective prove that the code paths are equivalently going to the same
place?

Which, given the complexity of the code, and the enormous ease with which
you can miss stuff (from personal experience...!), this is probably a
sensible way of doing it.

>  	if (aux_mutex)
>  		mutex_unlock(aux_mutex);
>
>