[PATCH] rlimit: locking tidy ups

[PATCH] rlimit: locking tidy ups

[Mateusz Guzik]

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 <mguzik@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(&current->sighand->siglock);
 		update_rlimit_cpu(current, rlimit(RLIMIT_CPU));
+		spin_unlock_irq(&current->sighand->siglock);
+		task_unlock(current);
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH] rlimit: locking tidy ups

[Eric W. Biederman]

Cc'd Oleg as he tends to be deeply involved with this class of locking.

Mateusz Guzik <mguzik@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 <mguzik@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(&current->sighand->siglock);
>  		update_rlimit_cpu(current, rlimit(RLIMIT_CPU));
> +		spin_unlock_irq(&current->sighand->siglock);
> +		task_unlock(current);
>  	}
>  }

Re: [PATCH] rlimit: locking tidy ups

[Oleg Nesterov]

On 05/04, Eric W. Biederman wrote:
>
> Cc'd Oleg as he tends to be deeply involved with this class of locking.
>
> Mateusz Guzik <mguzik@redhat.com> writes:
>
> > proc_pid_limits takes ->sighand lock prior to accessing rlimits, but it
> > serves no purpose as it does not prevent modifications.

Well. I agree this all needs cleanups or at least additional comments, but

> > @@ -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);

This is already unsafe. ->group_leader can point to nowhere if this threads
exits. lock_task_sighand() ensures that this can't happen.

> > -	/* protect tsk->signal and tsk->sighand from disappearing */
> > -	read_lock(&tasklist_lock);
> > -	if (!tsk->sighand) {
> > -		retval = -ESRCH;
> > -		goto out;
> > +	task_lock(tsk->group_leader);

The same, but yes the comment is misleading.

Oleg.