Received-SPF: pass (google.com: domain of linux-kernel+bounces-118018-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 25 Mar 2024 21:13:05 +0000
From: Al Viro <viro@zeniv.linux.org.uk>
To: Paulo Alcantara <pc@manguebit.com>
Cc: Steve French <smfrench@gmail.com>,
	Christian Brauner <brauner@kernel.org>,
	Roberto Sassu <roberto.sassu@huawei.com>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	CIFS <linux-cifs@vger.kernel.org>,
	Christian Brauner <christian@brauner.io>,
	Mimi Zohar <zohar@linux.ibm.com>, Paul Moore <paul@paul-moore.com>,
	"linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" <linux-security-module@vger.kernel.org>
Subject: Re: kernel crash in mknod
Message-ID: <20240325211305.GY538574@ZenIV>
References: <CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com>
 <20240324054636.GT538574@ZenIV>
 <3441a4a1140944f5b418b70f557bca72@huawei.com>
 <20240325-beugen-kraftvoll-1390fd52d59c@brauner>
 <CAH2r5muL4NEwLxq_qnPOCTHunLB_vmDA-1jJ152POwBv+aTcXg@mail.gmail.com>
 <20240325195413.GW538574@ZenIV>
 <a5d0ee8c54ec2f80cb71cd72e3b4aec3@manguebit.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a5d0ee8c54ec2f80cb71cd72e3b4aec3@manguebit.com>
Sender: Al Viro <viro@ftp.linux.org.uk>

On Mon, Mar 25, 2024 at 05:47:16PM -0300, Paulo Alcantara wrote:
> Al Viro <viro@zeniv.linux.org.uk> writes:
> 
> > On Mon, Mar 25, 2024 at 11:26:59AM -0500, Steve French wrote:
> >
> >> A loosely related question.  Do I need to change cifs.ko to return the
> >> pointer to inode on mknod now?  dentry->inode is NULL in the case of mknod
> >> from cifs.ko (and presumably some other fs as Al noted), unlike mkdir and
> >> create where it is filled in.   Is there a perf advantage in filling in the
> >> dentry->inode in the mknod path in the fs or better to leave it as is?  Is
> >> there a good example to borrow from on this?
> >
> > AFAICS, that case in in CIFS is the only instance of ->mknod() that does this
> > "skip lookups, just unhash and return 0" at the moment.
> >
> > What's more, it really had been broken all along for one important case -
> > AF_UNIX bind(2) with address (== socket pathname) being on the filesystem
> > in question.
> 
> Yes, except that we currently return -EPERM for such cases.  I don't
> even know if this SFU thing supports sockets.

	Sure, but we really want the rules to be reasonably simple and
"you may leave dentry unhashed negative and return 0, provided that you
hadn't been asked to create a socket" is anything but ;-)

> > Note that cifs_sfu_make_node() is the only case in CIFS where that happens -
> > other codepaths (both in cifs_make_node() and in smb2_make_node()) will
> > instantiate.  How painful would it be for cifs_sfu_make_node()?
> > AFAICS, you do open/sync_write/close there; would it be hard to do
> > an eqiuvalent of fstat and set the inode up?
> 
> This should be pretty straightforward as it would only require an extra
> query info call and then {smb311_posix,cifs}_get_inode_info() ->
> d_instantiate().  We could even make it a single compound request of
> open/write/getinfo/close for SMB2+ case.

	If that's the case, I believe that we should simply declare that
->mknod() must instantiate on success and have vfs_mknod() check and
warn if it hadn't.

	Rationale:

1) mknod(2) is usually followed by at least some access to created object.
Not setting the inode up won't save much anyway.
2) if some instance of ->mknod() skips setting the inode on success (i.e.
unhashes the still-negative dentry and returns 0), it can easily be
converted.  The minimal conversion would be along the lines of turning
	d_drop(dentry);
	return 0;
into
	d_drop(dentry);
	d = foofs_lookup(dir, dentry, 0);
	if (unlikely(d)) {
		if (!IS_ERR(d)) {
			dput(d);
			return -EINVAL;	// weird shit - directory got created somehow
		}
		return PTR_ERR(d);
	}
	return 0;
but there almost certainly are cheaper ways to get the inode metadata,
set the inode up and instantiate the dentry.
3) currently only on in-kernel instance is that way.
4) it makes life simpler for the users of vfs_mknod().

	Objections, anyone?