linux-kernel - Re: [PATCH] kernel/sys: Optimize do_prlimit lock scope to reduce contention

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: <20241127174536.752def18058e84487ab9ad65@linux-foundation.org>
Date: Wed, 27 Nov 2024 17:45:36 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Zhen Ni <zhen.ni@...ystack.cn>
Cc: viro@...iv.linux.org.uk, oleg@...hat.com, catalin.marinas@....com,
 brauner@...nel.org, zev@...ilderbeest.net, linux-kernel@...r.kernel.org,
 linux-security-module@...r.kernel.org
Subject: Re: [PATCH] kernel/sys: Optimize do_prlimit lock scope to reduce
 contention

On Wed, 20 Nov 2024 21:21:56 +0800 Zhen Ni <zhen.ni@...ystack.cn> wrote:

> Refines the lock scope in the do_prlimit function to reduce
> contention on task_lock(tsk->group_leader). The lock now protects only
> sections that access or modify shared resources (rlim). Permission
> checks (capable) and security validations (security_task_setrlimit)
> are placed outside the lock, as they do not modify rlim and are
> independent of shared data protection.

Let's cc linux-security-module@...r.kernel.org, as we're proposing
altering their locking environment!

> The security_task_setrlimit function is a Linux Security Module (LSM)
> hook that evaluates resource limit changes based on security policies.
> It does not alter the rlim data structure, as confirmed by existing
> LSM implementations (e.g., SELinux and AppArmor). Thus, this function
> does not require locking, ensuring correctness while improving
> concurrency.

Seems sane.

Does any code call do_prlimit() frequently enough for this to matter?

> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1481,18 +1481,20 @@ static int do_prlimit(struct task_struct *tsk, unsigned int resource,
>  
>  	/* Holding a refcount on tsk protects tsk->signal from disappearing. */
>  	rlim = tsk->signal->rlim + resource;
> -	task_lock(tsk->group_leader);
>  	if (new_rlim) {
>  		/*
>  		 * Keep the capable check against init_user_ns until cgroups can
>  		 * contain all limits.
>  		 */
>  		if (new_rlim->rlim_max > rlim->rlim_max &&
> -				!capable(CAP_SYS_RESOURCE))
> -			retval = -EPERM;
> -		if (!retval)
> -			retval = security_task_setrlimit(tsk, resource, new_rlim);
> +		    !capable(CAP_SYS_RESOURCE))
> +			return -EPERM;
> +		retval = security_task_setrlimit(tsk, resource, new_rlim);
> +		if (retval)
> +			return retval;
>  	}
> +
> +	task_lock(tsk->group_leader);
>  	if (!retval) {
>  		if (old_rlim)
>  			*old_rlim = *rlim;