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
Date: Mon, 25 Mar 2024 21:13:05 +0000 [thread overview]
Message-ID: <20240325211305.GY538574@ZenIV> (raw)
In-Reply-To: <a5d0ee8c54ec2f80cb71cd72e3b4aec3@manguebit.com>
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?
next prev parent reply other threads:[~2024-03-25 21:13 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com>
[not found] ` <20240324054636.GT538574@ZenIV>
2024-03-24 16:50 ` kernel crash in mknod Roberto Sassu
2024-03-24 21:02 ` Al Viro
2024-03-25 16:06 ` Christian Brauner
2024-03-25 17:18 ` Roberto Sassu
2024-03-26 11:40 ` Christian Brauner
2024-03-26 12:53 ` Paul Moore
2024-03-28 10:53 ` Roberto Sassu
2024-03-28 11:08 ` Christian Brauner
2024-03-28 11:24 ` Roberto Sassu
2024-03-28 12:07 ` Christian Brauner
2024-03-28 13:03 ` Paul Moore
2024-03-28 12:43 ` Paul Moore
2024-03-25 17:21 ` Paul Moore
[not found] ` <CAH2r5muL4NEwLxq_qnPOCTHunLB_vmDA-1jJ152POwBv+aTcXg@mail.gmail.com>
2024-03-25 19:54 ` Al Viro
2024-03-25 20:46 ` Al Viro
2024-03-25 20:47 ` Paulo Alcantara
2024-03-25 21:13 ` Al Viro [this message]
2024-03-25 21:31 ` Paulo Alcantara
2024-03-25 17:05 ` Paul Moore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240325211305.GY538574@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=brauner@kernel.org \
--cc=christian@brauner.io \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=pc@manguebit.com \
--cc=roberto.sassu@huawei.com \
--cc=smfrench@gmail.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).