linux-kernel - Re: [PATCH v3] apparmor: global buffers spin lock may get contended

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: <Y+9aoFjrYkpFSvuE@linutronix.de>
Date:   Fri, 17 Feb 2023 11:44:48 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     John Johansen <john.johansen@...onical.com>
Cc:     LKLM <linux-kernel@...r.kernel.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Tomasz Figa <tfiga@...omium.org>,
        linux-security-module@...r.kernel.org,
        Anil Altinay <aaltinay@...gle.com>
Subject: Re: [PATCH v3] apparmor: global buffers spin lock may get contended

On 2023-02-16 16:08:10 [-0800], John Johansen wrote:
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -49,12 +49,19 @@ union aa_buffer {
>  	char buffer[1];
>  };
> +struct aa_local_cache {
> +	unsigned int contention;
> +	unsigned int hold;
> +	struct list_head head;
> +};

if you stick a local_lock_t into that struct, then you could replace
	cache = get_cpu_ptr(&aa_local_buffers);
with
	local_lock(&aa_local_buffers.lock);
	cache = this_cpu_ptr(&aa_local_buffers);

You would get the preempt_disable() based locking for the per-CPU
variable (as with get_cpu_ptr()) and additionally some lockdep
validation which would warn if it is used outside of task context (IRQ).

I didn't parse completely the hold/contention logic but it seems to work
;)
You check "cache->count >=  2" twice but I don't see an inc/ dec of it
nor is it part of aa_local_cache.

I can't parse how many items can end up on the local list if the global
list is locked. My guess would be more than 2 due the ->hold parameter.

Do you have any numbers on the machine and performance it improved? It
sure will be a good selling point.

Sebastian