From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754625AbcEDSlK (ORCPT <rfc822;w@1wt.eu>);
	Wed, 4 May 2016 14:41:10 -0400
Received: from mx1.redhat.com ([209.132.183.28]:58877 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753307AbcEDSlI (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 4 May 2016 14:41:08 -0400
Date: Wed, 4 May 2016 19:39:11 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Mateusz Guzik <mguzik@redhat.com>, linux-kernel@vger.kernel.org,
        Eric Paris <eparis@parisplace.org>,
        James Morris <james.l.morris@oracle.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] rlimit: locking tidy ups
Message-ID: <20160504173910.GA1843@redhat.com>
References: <1462377062-16608-1-git-send-email-mguzik@redhat.com> <87r3dh4v6z.fsf@x220.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87r3dh4v6z.fsf@x220.int.ebiederm.org>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.