Re: [PATCH v2] proc: Allow pid_revalidate() during LOOKUP_RCU

From: ebiederm@xmission.com (Eric W. Biederman)
To: Stephen Brennan <stephen.s.brennan@oracle.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	linux-security-module@vger.kernel.org,
	Paul Moore <paul@paul-moore.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	selinux@vger.kernel.org, Casey Schaufler <casey@schaufler-ca.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v2] proc: Allow pid_revalidate() during LOOKUP_RCU
Date: Sun, 13 Dec 2020 08:30:40 -0600	[thread overview]
Message-ID: <87tusplqwf.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20201204000212.773032-1-stephen.s.brennan@oracle.com> (Stephen Brennan's message of "Thu, 3 Dec 2020 16:02:12 -0800")

Stephen Brennan <stephen.s.brennan@oracle.com> writes:

> The pid_revalidate() function requires dropping from RCU into REF lookup
> mode. When many threads are resolving paths within /proc in parallel,
> this can result in heavy spinlock contention as each thread tries to
> grab a reference to the /proc dentry lock (and drop it shortly
> thereafter).

I am feeling dense at the moment.  Which lock specifically are you
referring to?  The only locks I can thinking of are sleeping locks,
not spinlocks.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ebea9501afb8..833d55a59e20 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1830,19 +1846,22 @@ static int pid_revalidate(struct dentry *dentry, unsigned int flags)
>  {
>  	struct inode *inode;
>  	struct task_struct *task;
> +	int rv = 0;
>  
> -	if (flags & LOOKUP_RCU)
> -		return -ECHILD;
> -
> -	inode = d_inode(dentry);
> -	task = get_proc_task(inode);
> -
> -	if (task) {
> -		pid_update_inode(task, inode);
> -		put_task_struct(task);
> -		return 1;
> +	if (flags & LOOKUP_RCU) {

Why do we need to test flags here at all?
Why can't the code simply take an rcu_read_lock unconditionally and just
pass flags into do_pid_update_inode?

> +		inode = d_inode_rcu(dentry);
> +		task = pid_task(proc_pid(inode), PIDTYPE_PID);
> +		if (task)
> +			rv = do_pid_update_inode(task, inode, flags);
> +	} else {
> +		inode = d_inode(dentry);
> +		task = get_proc_task(inode);
> +		if (task) {
> +			rv = do_pid_update_inode(task, inode, flags);
> +			put_task_struct(task);
> +		}

>  	}
> -	return 0;
> +	return rv;
>  }
>  
>  static inline bool proc_inode_is_dead(struct inode *inode)

Eric