Cc'd Oleg as he tends to be deeply involved with this class of locking.
Mateusz Guzik <mgu...@redhat.com> writes:
> rlimits are stored in task->signal and are guaranteed to remain valid as
> long as the task struct is valid. All modifications are protected by
> locking task->group_leader. Additionally changes to RLIMIT_CPU need
> task->sighand.
>
> do_prlimit takes tasklist_lock, which as a side effect gurantees stable
> ->sighand however, there is no need to take the lock for any limit other
> than RLIMIT_CPU and even then we can get away with locking sighand itself.
>
> proc_pid_limits takes ->sighand lock prior to accessing rlimits, but it
> serves no purpose as it does not prevent modifications.
>
> Both functions effectively always perform ->sighand != NULL check, but it
> is only of concern when RLIMIT_CPU is being set. ->sighand is only cleared
> when the process is reaped, so a dedicated check only makes it less likely
> to access limits of a dead process.
>
> As such, eliminate the unneeded check and:
> - do_prlimit: stop taking tasklist_lock at all and only lock sighand when
> necessary
> - proc_pid_limits: lock group leader in order to obtain a stable copy
>
> Signed-off-by: Mateusz Guzik <mgu...@redhat.com>
> ---
> fs/proc/base.c | 6 ++----
> kernel/sys.c | 22 ++++++++++++++--------
> kernel/time/posix-cpu-timers.c | 3 +--
> security/selinux/hooks.c | 4 +++-
> 4 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 704ae63..3d4963e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -618,14 +618,12 @@ static int proc_pid_limits(struct seq_file *m, struct
> pid_namespace *ns,
> struct pid *pid, struct task_struct *task)
> {
> unsigned int i;
> - unsigned long flags;
>
> struct rlimit rlim[RLIM_NLIMITS];
>
> - if (!lock_task_sighand(task, &flags))
> - return 0;
> + task_lock(task->group_leader);
> memcpy(rlim, task->signal->rlim, sizeof(struct rlimit) * RLIM_NLIMITS);
> - unlock_task_sighand(task, &flags);
> + task_unlock(task->group_leader);
>
> /*
> * print the file header
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 89d5be4..1c8a67d 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1361,7 +1361,9 @@ int do_prlimit(struct task_struct *tsk, unsigned int
> resource,
> struct rlimit *new_rlim, struct rlimit *old_rlim)
> {
> struct rlimit *rlim;
> + unsigned long flags;
> int retval = 0;
> + int sighand_locked = 0;
>
> if (resource >= RLIM_NLIMITS)
> return -EINVAL;
> @@ -1373,15 +1375,17 @@ int do_prlimit(struct task_struct *tsk, unsigned int
> resource,
> return -EPERM;
> }
>
> - /* protect tsk->signal and tsk->sighand from disappearing */
> - read_lock(&tasklist_lock);
> - if (!tsk->sighand) {
> - retval = -ESRCH;
> - goto out;
> + task_lock(tsk->group_leader);
> + if (new_rlim && resource == RLIMIT_CPU &&
> + new_rlim->rlim_cur != RLIM_INFINITY) {
> + if (!lock_task_sighand(tsk, &flags)) {
> + retval = -ESRCH;
> + goto out;
> + }
> + sighand_locked = 1;
> }
>
> 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 */
> @@ -1407,7 +1411,6 @@ int do_prlimit(struct task_struct *tsk, unsigned int
> resource,
> if (new_rlim)
> *rlim = *new_rlim;
> }
> - task_unlock(tsk->group_leader);
>
> /*
> * RLIMIT_CPU handling. Note that the kernel fails to return an error
> @@ -1418,8 +1421,11 @@ int do_prlimit(struct task_struct *tsk, unsigned int
> resource,
> if (!retval && new_rlim && resource == RLIMIT_CPU &&
> new_rlim->rlim_cur != RLIM_INFINITY)
> update_rlimit_cpu(tsk, new_rlim->rlim_cur);
> +
> + if (sighand_locked)
> + unlock_task_sighand(tsk, &flags);
> out:
> - read_unlock(&tasklist_lock);
> + task_unlock(tsk->group_leader);
> return retval;
> }
>
> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 1cafba8..fc38417 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -23,9 +23,8 @@ void update_rlimit_cpu(struct task_struct *task, unsigned
> long rlim_new)
> {
> cputime_t cputime = secs_to_cputime(rlim_new);
>
> - spin_lock_irq(&task->sighand->siglock);
> + lockdep_assert_held(&task->sighand->siglock);
> set_process_cpu_timer(task, CPUCLOCK_PROF, &cputime, NULL);
> - spin_unlock_irq(&task->sighand->siglock);
> }
>
> static int check_clock(const clockid_t which_clock)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..d74b91a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2506,8 +2506,10 @@ static void selinux_bprm_committing_creds(struct
> linux_binprm *bprm)
> initrlim = init_task.signal->rlim + i;
> rlim->rlim_cur = min(rlim->rlim_max,
> initrlim->rlim_cur);
> }
> - task_unlock(current);
> + spin_lock_irq(¤t->sighand->siglock);
> update_rlimit_cpu(current, rlimit(RLIMIT_CPU));
> + spin_unlock_irq(¤t->sighand->siglock);
> + task_unlock(current);
> }
> }
