linux-kernel - Re: [PATCH] rlimits: do not grab tasklist_lock for do

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: <8735mww2w3.fsf@email.froward.int.ebiederm.org>
Date:   Mon, 13 Dec 2021 16:34:04 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Barret Rhoden <brho@...gle.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexey Gladkov <legion@...nel.org>,
        William Cohen <wcohen@...hat.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Chris Hyser <chris.hyser@...cle.com>,
        Peter Collingbourne <pcc@...gle.com>,
        Xiaofeng Cao <caoxiaofeng@...ong.com>,
        David Hildenbrand <david@...hat.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rlimits: do not grab tasklist_lock for do_prlimit on current

Barret Rhoden <brho@...gle.com> writes:

> The tasklist_lock can be a scalability bottleneck.  For current tasks,
> we don't need the tasklist_lock to protect tsk->sighand or tsk->signal.
> If non-current callers become a bottleneck, we could use
> lock_task_sighand().

Do you have any numbers?  As the entire point of this change is
performance it would be good to see how the performance changes.

Especially as a read_lock should not be too bad as it allows sharing,
nor do I expect reading or writing the rlimit values to be particularly
frequent.  So some insight into what kinds of userspace patterns make
this a problem would be nice.

This change is a bit scary as it makes taking a lock conditional and
increases the probability of causing a locking mistake.

If you are going to make this change I would say that do_prlimit should
become static and taking the tasklist_lock should move into prlimit64.

Looking a little closer it looks like that update_rlimit_cpu should use
lock_task_sighand, and once lock_task_sighand is used there is actually
no need for the tasklist_lock at all.  As holding the reference to tsk
guarantees that tsk->signal remains valid.

So I completely agree there are cleanups that can happen in this area.
Please make those and show numbers in how they improve things, instead
of making the code worse with a conditional lock.

Eric

> Signed-off-by: Barret Rhoden <brho@...gle.com>
> ---
>  kernel/sys.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8fdac0d90504..e56d1ae910af 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1576,7 +1576,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>  	}
>  
>  	/* protect tsk->signal and tsk->sighand from disappearing */
> -	read_lock(&tasklist_lock);
> +	if (tsk != current)
> +		read_lock(&tasklist_lock);
>  	if (!tsk->sighand) {
>  		retval = -ESRCH;
>  		goto out;
> @@ -1611,7 +1612,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>  	     IS_ENABLED(CONFIG_POSIX_TIMERS))
>  		update_rlimit_cpu(tsk, new_rlim->rlim_cur);
>  out:
> -	read_unlock(&tasklist_lock);
> +	if (tsk != current)
> +		read_unlock(&tasklist_lock);
>  	return retval;
>  }