Re: [RFC 11/18] limits: track and present RLIMIT_NPROC actual max

[Jann Horn <jann@thejh.net>]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Tue, Jun 14, 2016 at 03:40:35PM +0000, Topi Miettinen wrote:
> On 06/13/16 22:27, Jann Horn wrote:
> > On Mon, Jun 13, 2016 at 10:44:18PM +0300, Topi Miettinen wrote:
> >> Track maximum number of processes per user and present it
> >> in /proc/self/limits.
> >>
> >> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
> >> ---
> >>  fs/proc/base.c        | 4 ++++
> >>  include/linux/sched.h | 1 +
> >>  kernel/fork.c         | 5 +++++
> >>  kernel/sys.c          | 5 +++++
> >>  4 files changed, 15 insertions(+)
> >>
> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> index 1df4fc8..02576c6 100644
> >> --- a/fs/proc/base.c
> >> +++ b/fs/proc/base.c
> >> @@ -670,6 +670,10 @@ static int proc_pid_limits(struct seq_file *m, struct pid_namespace *ns,
> >>  				seq_printf(m, "%-20lu\n", psecs);
> >>  			}
> >>  			break;
> >> +		case RLIMIT_NPROC:
> >> +			seq_printf(m, "%-20d\n",
> >> +				   atomic_read(&task->real_cred->user->max_processes));
> > 
> > Don't you have to take an RCU read lock before dereferencing task->real_cred?
> 
> In other comments in the series, cmpxchg loop was suggested, would that
> work here?

What would a cmpxchg loop have to do with missing RCU locking?

> > And shouldn't this be done with __task_cred(task) instead of task->real_cred?
> 
> How about atomic_read(task_cred_xxx(task, user)->max_processes)?

No. You'd still end up dereferencing max_processes in the user_struct without
any guarantee that it hasn't been freed. I think the code should look this way:

    case RLIMIT_NPROC:
        rcu_read_lock();
        seq_printf(m, "%-20d\n",
            atomic_read(&__task_cred(task)->user->max_processes));
        rcu_read_unlock();
        break;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]