From: ebiederm@xmission.com (Eric W. Biederman)
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Andy Lutomirski <luto@amacapital.net>, security@debian.org,
        "security\@kernel.org" <security@kernel.org>,
        "security\@ubuntu.com \>\> security" <security@ubuntu.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        Serge Hallyn <serge.hallyn@ubuntu.com>, Willy Tarreau <w@1wt.eu>,
        Aurelien Jarno <aurelien@aurel32.net>,
        One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
        Jann Horn <jann@thejh.net>, Greg KH <greg@kroah.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jiri Slaby <jslaby@suse.com>, Florian Weimer <fw@deneb.enyo.de>
References: <CA+55aFzs00iDkYhvFCq=AZMVcNL0+oZT4SeimTeVurJq=5ZS3A@mail.gmail.com>
	<CALCETrUQ_Oad61dYvPpYFxDBT++jhZoAT3e4n4yiu22iTNcxNQ@mail.gmail.com>
	<CALCETrUJ_KuGJoVxuUjWGCNHBNs5Vk8E68MtsWoTdURJ+=iJnQ@mail.gmail.com>
	<CA+55aFzchGpOp0R0RjM6Bsk23GWp1t76fgFi8q6FZQndHq5EtA@mail.gmail.com>
	<570D4781.3070600@zytor.com> <877ffyzy1j.fsf_-_@x220.int.ebiederm.org>
	<alpine.LFD.2.20.1604161105040.30857@i7>
	<87twixgsnq.fsf@x220.int.ebiederm.org>
	<CA+55aFyOv3jUOsC-JbfeqM-Rs1z1a+QEt2EejpTf-9EfXdttdg@mail.gmail.com>
	<87oa95gevf.fsf_-_@x220.int.ebiederm.org>
	<20160420032541.GE25498@ZenIV.linux.org.uk>
Date: Tue, 19 Apr 2016 22:43:03 -0500
In-Reply-To: <20160420032541.GE25498@ZenIV.linux.org.uk> (Al Viro's message of
	"Wed, 20 Apr 2016 04:25:42 +0100")
Message-ID: <87k2jtc5dk.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
Subject: Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1442
Lines: 51

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Tue, Apr 19, 2016 at 10:04:20PM -0500, Eric W. Biederman wrote:
>> +#ifdef CONFIG_UNIX98_PTYS
>> +int path_pts(struct path *path)
>> +{
>> +	/* Find "pts" in the same directory as the input path */
>> +	struct dentry *child, *parent;
>> +	struct qstr this;
>> +	int ret;
>> +
>> +	ret = path_parent_directory(path);
>> +	if (ret)
>> +		return ret;
>> +
>> +	parent = path->dentry;
>> +	if (!d_can_lookup(parent))
>> +		return -ENOENT;
>
> And how, pray tell, would a parent of anything fail to be a directory?

It is to make that function be visually distinct from path_parentat
which does something rather different. 

>> +	this.name = "pts";
>> +	this.len = 3;
>> +	this.hash = full_name_hash(this.name, this.len);
>> +	if (parent->d_flags & DCACHE_OP_HASH) {
>> +		int err = parent->d_op->d_hash(parent, &this);
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +	inode_lock(parent->d_inode);
>
> What the hell for?  What does that lock on parent change for the
> dcache lookup you are doing here?

Good point. That is overkill. As we know the dentry is a mount point and
must be in the dcache, the customary lock for performing a lookup from
the disk is not necessary.

>> +	child = d_lookup(parent, &this);
>> +	inode_unlock(parent->d_inode);
>> +	if (!child)
>> +		return -ENOENT;
>
> Take a look at d_hash_and_lookup(), BTW...

Yes.  That does look like a reasonable simplification.

Eric