Received-SPF: pass (google.com: domain of linux-kernel+bounces-118044-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <072a3e1b9f40a44174f0738bf592a528@manguebit.com>
From: Paulo Alcantara <pc@manguebit.com>
To: Al Viro <viro@zeniv.linux.org.uk>, Steve French <smfrench@gmail.com>
Cc: 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
In-Reply-To: <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>
 <20240325211305.GY538574@ZenIV>
Date: Mon, 25 Mar 2024 18:31:42 -0300
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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

> 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 ;-)

Agreed :-)

>> > 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.

LGTM.

Steve, any objections?