From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754833AbcEDRI6 (ORCPT ); Wed, 4 May 2016 13:08:58 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:52153 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753385AbcEDRIz (ORCPT ); Wed, 4 May 2016 13:08:55 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Mateusz Guzik Cc: linux-kernel@vger.kernel.org, Eric Paris , James Morris , Thomas Gleixner , Al Viro , Oleg Nesterov References: <1462377062-16608-1-git-send-email-mguzik@redhat.com> Date: Wed, 04 May 2016 11:57:56 -0500 In-Reply-To: <1462377062-16608-1-git-send-email-mguzik@redhat.com> (Mateusz Guzik's message of "Wed, 4 May 2016 17:51:02 +0200") Message-ID: <87r3dh4v6z.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1+cRwQjDCtiZuX8B1JUbiHdZKagathSl5I= X-SA-Exim-Connect-IP: 97.119.108.118 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 1.0 T_XMDrugObfuBody_08 obfuscated drug references * 0.2 T_XMDrugObfuBody_14 obfuscated drug references X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: *;Mateusz Guzik X-Spam-Relay-Country: X-Spam-Timing: total 548 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 3.6 (0.7%), b_tie_ro: 2.6 (0.5%), parse: 0.90 (0.2%), extract_message_metadata: 5 (0.9%), get_uri_detail_list: 3.3 (0.6%), tests_pri_-1000: 3.6 (0.7%), tests_pri_-950: 1.27 (0.2%), tests_pri_-900: 1.01 (0.2%), tests_pri_-400: 30 (5.6%), check_bayes: 29 (5.4%), b_tokenize: 9 (1.7%), b_tok_get_all: 8 (1.5%), b_comp_prob: 2.4 (0.4%), b_tok_touch_all: 6 (1.0%), b_finish: 2.0 (0.4%), tests_pri_0: 488 (89.2%), check_dkim_signature: 0.56 (0.1%), check_dkim_adsp: 3.2 (0.6%), tests_pri_500: 3.7 (0.7%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] rlimit: locking tidy ups X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Cc'd Oleg as he tends to be deeply involved with this class of locking. Mateusz Guzik 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 > --- > 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); > } > }