LinuxLists.cc - kernel crash in mknod

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Sun, Mar 24, 2024 at 12:00:15AM -0500, Steve French wrote:
> Anyone else seeing this kernel crash in do_mknodat (I see it with a
> simple "mkfifo" on smb3 mount). I started seeing this in 6.9-rc (did
> not see it in 6.8). I did not see it with the 3/12/23 mainline
> (early in the 6.9-rc merge Window) but I do see it in the 3/22 build
> so it looks like the regression was introduced by:

FWIW, successful ->mknod() is allowed to return 0 and unhash
dentry, rather than bothering with lookups. So commit in question
is bogus - lack of error does *NOT* mean that you have struct inode
existing, let alone attached to dentry. That kind of behaviour
used to be common for network filesystems more than just for ->mknod(),
the theory being "if somebody wants to look at it, they can bloody
well pay the cost of lookup after dcache miss".

Said that, the language in D/f/vfs.rst is vague as hell and is very easy
to misread in direction of "you must instantiate".

Thankfully, there's no counterpart with mkdir - *there* it's not just
possible, it's inevitable in some cases for e.g. nfs.

What the hell is that hook doing in non-S_IFREG cases, anyway? Move it
up and be done with it...

diff --git a/fs/namei.c b/fs/namei.c
index ceb9ddf8dfdd..821fe0e3f171 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4050,6 +4050,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
case 0: case S_IFREG:
error = vfs_create(idmap, path.dentry->d_inode,
dentry, mode, true);
+ if (!error)
+ error = security_path_post_mknod(idmap, dentry);
break;
case S_IFCHR: case S_IFBLK:
error = vfs_mknod(idmap, path.dentry->d_inode,
@@ -4061,10 +4063,6 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
break;
}

- if (error)
- goto out2;
-
- security_path_post_mknod(idmap, dentry);
out2:
done_path_create(&path, dentry);
if (retry_estale(error, lookup_flags)) {

2024-03-24 08:56:58

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Sun, Mar 24, 2024 at 05:46:36AM +0000, Al Viro wrote:
> On Sun, Mar 24, 2024 at 12:00:15AM -0500, Steve French wrote:
> > Anyone else seeing this kernel crash in do_mknodat (I see it with a
> > simple "mkfifo" on smb3 mount). I started seeing this in 6.9-rc (did
> > not see it in 6.8). I did not see it with the 3/12/23 mainline
> > (early in the 6.9-rc merge Window) but I do see it in the 3/22 build
> > so it looks like the regression was introduced by:
>
> FWIW, successful ->mknod() is allowed to return 0 and unhash
> dentry, rather than bothering with lookups. So commit in question
> is bogus - lack of error does *NOT* mean that you have struct inode
> existing, let alone attached to dentry. That kind of behaviour
> used to be common for network filesystems more than just for ->mknod(),
> the theory being "if somebody wants to look at it, they can bloody
> well pay the cost of lookup after dcache miss".
>
> Said that, the language in D/f/vfs.rst is vague as hell and is very easy
> to misread in direction of "you must instantiate".
>
> Thankfully, there's no counterpart with mkdir - *there* it's not just
> possible, it's inevitable in some cases for e.g. nfs.
>
> What the hell is that hook doing in non-S_IFREG cases, anyway? Move it
> up and be done with it...

PS: moving the call site up to S_IFREG case deals with the immediate
problem (->create() *IS* required to make dentry uptodate), but the other
side of what had lead to that bug needs to be dealt with separately.

It needs to be documented clearly (for all object-creating methods) and
we need to decide what their behaviours should be. Right now it's
successful ->create() must make positive
successful ->mkdir() may leave negative unhashed (and it might be forced to)
successful ->tmpfile() must make positive
->mknod() and ->symlink() are uncertain. VFS doesn't give a damn;
other users might. FWIW, ecryptfs is fine with either behaviour.
nfsd and overlayfs might or might not break. AF_UNIX bind()
probably *does* break on such ->mknod() behaviour and unless I'm
misreading the history it had been that way since way back.

From a quick look through ->mknod() instances it looks like
CIFS_MOUNT_UNX_EMUL case in cifs is the odd man out at the moment.

Could you check it AF_UNIX bind() with ->sun_path containing
a pathname that resolves to (inexistent) file on your filesystem
breaks with your setup?
setup?

2024-03-24 23:31:32

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Sun, Mar 24, 2024 at 04:50:24PM +0000, Roberto Sassu wrote:

> Also, please update the description of security_path_post_mknod() to say
> that it is not going to be called for non-regular files.

If anything, it's rather security_past_create_without_open(), and
I really wonder where does the equivalent of those actions happen
if you do close(open("foo", O_CREAT|O_RDWR, 0777)) instead of
mknod("foo", 0777, 0). I mean, you can substitute the former
for the latter, so anything that must be done by the hook in
mknod(2) would better be covered at some point in hooks within
open(2)... Some explanation of the relationship between those
would be nice.

2024-03-24 16:50:51

[permalink] [raw]

Subject: RE: kernel crash in mknod

> From: Al Viro [mailto:[email protected]] On Behalf Of Al Viro
> Sent: Sunday, March 24, 2024 6:47 AM
> On Sun, Mar 24, 2024 at 12:00:15AM -0500, Steve French wrote:
> > Anyone else seeing this kernel crash in do_mknodat (I see it with a
> > simple "mkfifo" on smb3 mount). I started seeing this in 6.9-rc (did
> > not see it in 6.8). I did not see it with the 3/12/23 mainline
> > (early in the 6.9-rc merge Window) but I do see it in the 3/22 build
> > so it looks like the regression was introduced by:
>
> FWIW, successful ->mknod() is allowed to return 0 and unhash
> dentry, rather than bothering with lookups. So commit in question
> is bogus - lack of error does *NOT* mean that you have struct inode
> existing, let alone attached to dentry. That kind of behaviour
> used to be common for network filesystems more than just for ->mknod(),
> the theory being "if somebody wants to look at it, they can bloody
> well pay the cost of lookup after dcache miss".
>
> Said that, the language in D/f/vfs.rst is vague as hell and is very easy
> to misread in direction of "you must instantiate".
>
> Thankfully, there's no counterpart with mkdir - *there* it's not just
> possible, it's inevitable in some cases for e.g. nfs.
>
> What the hell is that hook doing in non-S_IFREG cases, anyway? Move it
> up and be done with it...

Hi Al

thanks for the patch. Indeed, it was like that before, when instead of
an LSM hook there was an IMA call.

However, I thought, since we were promoting it as an LSM hook,
we should be as generic possible, and support more usages than
what was needed for IMA.

> diff --git a/fs/namei.c b/fs/namei.c
> index ceb9ddf8dfdd..821fe0e3f171 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4050,6 +4050,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> case 0: case S_IFREG:
> error = vfs_create(idmap, path.dentry->d_inode,
> dentry, mode, true);
> + if (!error)
> + error = security_path_post_mknod(idmap, dentry);

Minor issue, security_path_post_mknod() does not return an error.

Also, please update the description of security_path_post_mknod() to say
that it is not going to be called for non-regular files.

Hopefully, Paul also agrees with this change.

Other than that, please add my:

Reviewed-by: Roberto Sassu <[email protected]>

Thanks

Roberto

> break;
> case S_IFCHR: case S_IFBLK:
> error = vfs_mknod(idmap, path.dentry->d_inode,
> @@ -4061,10 +4063,6 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> break;
> }
>
> - if (error)
> - goto out2;
> -
> - security_path_post_mknod(idmap, dentry);
> out2:
> done_path_create(&path, dentry);
> if (retry_estale(error, lookup_flags)) {

2024-03-25 17:21:35

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Sun, Mar 24, 2024 at 04:50:24PM +0000, Roberto Sassu wrote:
> > From: Al Viro [mailto:[email protected]] On Behalf Of Al Viro
> > Sent: Sunday, March 24, 2024 6:47 AM
> > On Sun, Mar 24, 2024 at 12:00:15AM -0500, Steve French wrote:
> > > Anyone else seeing this kernel crash in do_mknodat (I see it with a
> > > simple "mkfifo" on smb3 mount). I started seeing this in 6.9-rc (did
> > > not see it in 6.8). I did not see it with the 3/12/23 mainline
> > > (early in the 6.9-rc merge Window) but I do see it in the 3/22 build
> > > so it looks like the regression was introduced by:
> >
> > FWIW, successful ->mknod() is allowed to return 0 and unhash
> > dentry, rather than bothering with lookups. So commit in question
> > is bogus - lack of error does *NOT* mean that you have struct inode
> > existing, let alone attached to dentry. That kind of behaviour
> > used to be common for network filesystems more than just for ->mknod(),
> > the theory being "if somebody wants to look at it, they can bloody
> > well pay the cost of lookup after dcache miss".
> >
> > Said that, the language in D/f/vfs.rst is vague as hell and is very easy
> > to misread in direction of "you must instantiate".
> >
> > Thankfully, there's no counterpart with mkdir - *there* it's not just
> > possible, it's inevitable in some cases for e.g. nfs.
> >
> > What the hell is that hook doing in non-S_IFREG cases, anyway? Move it
> > up and be done with it...
>
> Hi Al
>
> thanks for the patch. Indeed, it was like that before, when instead of
> an LSM hook there was an IMA call.

Could you please start adding lore links into your commit messages for
all messages that are sent to a mailing list? It really makes tracking
down the original thread a lot easier.

> However, I thought, since we were promoting it as an LSM hook,
> we should be as generic possible, and support more usages than
> what was needed for IMA.

I'm a bit confused now why this is taking a dentry. Nothing in IMA or
EVM cares about the dentry for these hooks so it really should have take
an inode in the first place?

And one minor other question I just realized. Why are some of the new
hooks called security_path_post_mknod() when they aren't actually taking
a path in contrast to say
security_path_{chown,chmod,mknod,chroot,truncate}() that do.

2024-03-25 17:58:07

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Sun, Mar 24, 2024 at 12:50 PM Roberto Sassu <[email protected]> wrote:
> > From: Al Viro [mailto:[email protected]] On Behalf Of Al Viro
> > Sent: Sunday, March 24, 2024 6:47 AM
> > On Sun, Mar 24, 2024 at 12:00:15AM -0500, Steve French wrote:
> > > Anyone else seeing this kernel crash in do_mknodat (I see it with a
> > > simple "mkfifo" on smb3 mount). I started seeing this in 6.9-rc (did
> > > not see it in 6.8). I did not see it with the 3/12/23 mainline
> > > (early in the 6.9-rc merge Window) but I do see it in the 3/22 build
> > > so it looks like the regression was introduced by:
> >
> > FWIW, successful ->mknod() is allowed to return 0 and unhash
> > dentry, rather than bothering with lookups. So commit in question
> > is bogus - lack of error does *NOT* mean that you have struct inode
> > existing, let alone attached to dentry. That kind of behaviour
> > used to be common for network filesystems more than just for ->mknod(),
> > the theory being "if somebody wants to look at it, they can bloody
> > well pay the cost of lookup after dcache miss".
> >
> > Said that, the language in D/f/vfs.rst is vague as hell and is very easy
> > to misread in direction of "you must instantiate".
> >
> > Thankfully, there's no counterpart with mkdir - *there* it's not just
> > possible, it's inevitable in some cases for e.g. nfs.
> >
> > What the hell is that hook doing in non-S_IFREG cases, anyway? Move it
> > up and be done with it...
>
> Hi Al
>
> thanks for the patch. Indeed, it was like that before, when instead of
> an LSM hook there was an IMA call.
>
> However, I thought, since we were promoting it as an LSM hook,
> we should be as generic possible, and support more usages than
> what was needed for IMA.
>
> > diff --git a/fs/namei.c b/fs/namei.c
> > index ceb9ddf8dfdd..821fe0e3f171 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4050,6 +4050,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > case 0: case S_IFREG:
> > error = vfs_create(idmap, path.dentry->d_inode,
> > dentry, mode, true);
> > + if (!error)
> > + error = security_path_post_mknod(idmap, dentry);
>
> Minor issue, security_path_post_mknod() does not return an error.
>
> Also, please update the description of security_path_post_mknod() to say
> that it is not going to be called for non-regular files.
>
> Hopefully, Paul also agrees with this change.
>
> Other than that, please add my:
>
> Reviewed-by: Roberto Sassu <[email protected]>

No objections here for obvious reasons.

Acked-by: Paul Moore <[email protected]>

--
paul-moore.com

2024-03-25 18:02:08

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Mon, Mar 25, 2024 at 12:06 PM Christian Brauner <[email protected]> wrote:
> I'm a bit confused now why this is taking a dentry. Nothing in IMA or
> EVM cares about the dentry for these hooks so it really should have take
> an inode in the first place?

I don't want to speak for Roberto or Mimi here, but this LSM hook was
intended to replace the dedicated ima_post_path_mknod() hook as I
wanted to see IMA/EVM integrated as proper LSMs so we could so away
with all of the special IMA/EVM hooks and treat everything as a LSM.
Part of this was creating new LSM hooks where historically we only had
a IMA and/or EVM hook, the security_path_post_mknod() hook is such a
case (e.g. /ima_post_path_mknod/security_path_post_mknod/) and the new
LSM hook kept the same parameters as the old IMA hook.

Yes, you are correct that neither IMA and EVM do anything with the
dentry other than looking at the associated inode. I'm not the
IMA/EVM expert in this thread, but I suspect this is simply an old
vestige of former code, or perhaps an "optimization" to avoid having
to fetch the inode pointer in cases where IMA/EVM was not enabled
(although it is used in the vfs_create() call directly above, so who
knows ...

> And one minor other question I just realized. Why are some of the new
> hooks called security_path_post_mknod() when they aren't actually taking
> a path in contrast to say
> security_path_{chown,chmod,mknod,chroot,truncate}() that do.

Once again, think of this as a
/ima_post_path_mknod/security_path_post_mknod/ type of replacement and
you have your answer. That said, I'm not really against bikeshedding
LSM hook names if people want to do that, it's not a stable protected
API so while we try to keep it stable~ish simply for our own sanity,
I'm happy to see it changed if everyone agrees it makes sense.

--
paul-moore.com

2024-03-25 19:55:23

[permalink] [raw]

Subject: Re: kernel crash in mknod

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.

Options:
1) make vfs_mknod() callers aware of the possibility, have the ones
that care do lookup in case when return value is 0 and dentry is unhashed.
That's similar to what we do for vfs_mkdir(). No changes needed for CIFS
or fs/namei.c (i.e. do_mknodat()), unix_bind() definitely needs a change,
ecryptfs can stay as-is, overlayfs just needs to stop complaining when it sees
that situation, nfsd might or might not need a change - hadn't checked yet.
In that case we document ->mknod() as "may unhash and return 0 if it wants
to save a lookup".
2) make vfs_mknod() check for that case and have it call ->lookup()
if it sees that. I don't see any benefits to that, TBH - no performance
benefits anywhere and no real simplification for ->mknod() instances. It
does avoid the need to change anything in CIFS, though.
3) require ->mknod() instances to make dentry positive on success.
CIFS needs a fix, documentation gets updated to explicitly require that.
AFAICS, nothing else would need to be touched, except possibly adding
a warning in vfs_mknod() to catch violation of that rule.

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? No need to reread the
file contents (as cifs_sfu_type() does), and you do have full path
anyway, so it's less work than for full ->lookup() even if you need
a path-based protocol operations...

Does that thing have an equivalent of fstat() that would return the
metadata of opened file?

2024-03-25 21:32:03

by Paulo Alcantara

[permalink] [raw]

Subject: Re: kernel crash in mknod

Al Viro <[email protected]> writes:

> On Mon, Mar 25, 2024 at 05:47:16PM -0300, Paulo Alcantara wrote:
>> Al Viro <[email protected]> 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?

2024-03-25 23:41:45

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Mon, Mar 25, 2024 at 05:47:16PM -0300, Paulo Alcantara wrote:
> Al Viro <[email protected]> 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?

2024-03-26 00:31:08

by Paulo Alcantara

[permalink] [raw]

Subject: Re: kernel crash in mknod

Al Viro <[email protected]> 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.

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

2024-03-26 00:52:40

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Mon, Mar 25, 2024 at 07:54:13PM +0000, Al Viro wrote:

> 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? No need to reread the
> file contents (as cifs_sfu_type() does), and you do have full path
> anyway, so it's less work than for full ->lookup() even if you need
> a path-based protocol operations...
>
> Does that thing have an equivalent of fstat() that would return the
> metadata of opened file?

You do have a FID there, so doing ->query_file_info() just before close,
using the result to build inode (with type and ->i_rdev taken from what
you've been given by the caller) and passing it to d_instantiate() looks
not entirely implausible, but I'm really not familiar with the codebase,
so take that with a cartload of salt.

mknod() usually is followed by lookup of some sort pretty soon, and your
lookup would have to do at least open/sync_read/close just to decode the
device number. So if anything, *not* setting an inode up during mknod()
is likely to be a pessimization...

If we did it in vfs_mknod() callers, that would be something along the
lines of
err = vfs_mknod(..., dir, dentry, ...)
if (err)
fuck off
if (unlikely(!dentry->d_inode)) {
if (d_unhashed(dentry)) {
struct dentry *d = dir->i_op->lookup(dir, dentry, 0);
if (unlikely(d)) {
if (IS_ERR(d)) {
fuck off, lookup failed
} else {
// ->lookup returns a pointer to existing
// alias *ONLY* for directories; WTF is
// going on?
dput(d);
fuck off, wrong thing created there
}
}
if (!dentry->d_inode)
fuck off, it hasn't been created
if (wrong type of dentry->d_inode))
fuck off, wrong thing created there
OK, there we go
} else {
complain about bogus ->mknod() behavior
fuck off - it hasn't been created, apparently
}
}
at least in net/unix/af_unix.c:unix_bind(). So the minimal change
would be to have your d_drop(dentry) in that codepath followed by
cifs_lookup(<parent inode>, dentry, 0) and checking the result.

But I would very much suspect that fetching metadata by fid before
you close the file would be cheaper than full-blown cifs_lookup()
there.

2024-03-26 11:40:38

[permalink] [raw]

Subject: Re: kernel crash in mknod

> we can change the parameter of security_path_post_mknod() from
> dentry to inode?

If all current callers only operate on the inode then it seems the best
to only pass the inode. If there's some reason someone later needs a
dentry the hook can always be changed.

For bigger changes it's also worthwhile if the object that's passed down
into the hook-based LSM layer is as specific as possible. If someone
does a change that affects lifetime rules of mounts then any hook that
takes a struct path argument that's unused means going through each LSM
that implements the hook only to find out it's not actually used.
Similar for dentry vs inode imho.

2024-03-26 12:53:39

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Tue, Mar 26, 2024 at 7:40 AM Christian Brauner <[email protected]> wrote:
>
> For bigger changes it's also worthwhile if the object that's passed down
> into the hook-based LSM layer is as specific as possible. If someone
> does a change that affects lifetime rules of mounts then any hook that
> takes a struct path argument that's unused means going through each LSM
> that implements the hook only to find out it's not actually used.
> Similar for dentry vs inode imho.

For bigger changes please always ensure that the LSM list, and any
related LSM implementation lists, are on the To/CC line. While we
appreciate Christian's input (and Al's, and all the other VFS devs) on
VFS matters, there are often other considerations that need to be
taken into account when discussing LSM related issues. Generally,
"specific as possible" is good input, but it isn't the only thing we
need to worry about, and sometimes other requirements mean that it
isn't the best choice. Just as we want the VFS devs involved in
discussions about VFS related LSM hooks (these new IMA/EVM-related LSM
hooks were sent to, and reviewed by the VFS folks), I would hope the
VFS devs would want to include the LSM devs on any LSM related issues
and would try to avoid speaking on behalf of the LSM devs and
maintainers.

--
paul-moore.com

2024-03-28 11:02:13

[permalink] [raw]

Subject: Re: kernel crash in mknod

On 3/26/2024 12:40 PM, Christian Brauner wrote:
>> we can change the parameter of security_path_post_mknod() from
>> dentry to inode?
>
> If all current callers only operate on the inode then it seems the best
> to only pass the inode. If there's some reason someone later needs a
> dentry the hook can always be changed.

Ok, so the crash is likely caused by:

void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
*dentry)
{
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))

I guess we can also simply check if there is an inode attached to the
dentry, to minimize the changes. I can do both.

More technical question, do I need to do extra checks on the dentry
before calling security_path_post_mknod()?

Thanks

Roberto

> For bigger changes it's also worthwhile if the object that's passed down
> into the hook-based LSM layer is as specific as possible. If someone
> does a change that affects lifetime rules of mounts then any hook that
> takes a struct path argument that's unused means going through each LSM
> that implements the hook only to find out it's not actually used.
> Similar for dentry vs inode imho.

2024-03-28 11:08:51

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Thu, Mar 28, 2024 at 12:53:40PM +0200, Roberto Sassu wrote:
> On 3/26/2024 12:40 PM, Christian Brauner wrote:
> > > we can change the parameter of security_path_post_mknod() from
> > > dentry to inode?
> >
> > If all current callers only operate on the inode then it seems the best
> > to only pass the inode. If there's some reason someone later needs a
> > dentry the hook can always be changed.
>
> Ok, so the crash is likely caused by:
>
> void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
> *dentry)
> {
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>
> I guess we can also simply check if there is an inode attached to the
> dentry, to minimize the changes. I can do both.
>
> More technical question, do I need to do extra checks on the dentry before
> calling security_path_post_mknod()?

Why do you need the dentry? The two users I see are ima in [1] and evm in [2].
Both of them don't care about the dentry. They only care about the
inode. So why is that hook not just:

diff --git a/security/security.c b/security/security.c
index 7e118858b545..025689a7e912 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1799,11 +1799,11 @@ EXPORT_SYMBOL(security_path_mknod);
*
* Update inode security field after a file has been created.
*/
-void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
+void security_inode_post_mknod(struct mnt_idmap *idmap, struct inode *inode)
{
- if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+ if (unlikely(IS_PRIVATE(inode)))
return;
- call_void_hook(path_post_mknod, idmap, dentry);
+ call_void_hook(path_post_mknod, idmap, inode);
}

/**

And one another thing I'd like to point out is that the security hook is
called "security_path_post_mknod()" while the evm and ima hooks are
called evm_post_path_mknod() and ima_post_path_mknod() respectively. In
other words:

git grep _path_post_mknod() doesn't show the implementers of that hook
which is rather unfortunate. It would be better if the pattern were:

<specific LSM>_$some_$ordered_$words()

[1]:
static void evm_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
{
struct inode *inode = d_backing_inode(dentry);
struct evm_iint_cache *iint = evm_iint_inode(inode);

if (!S_ISREG(inode->i_mode))
return;

if (iint)
iint->flags |= EVM_NEW_FILE;
}

[2]:
static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
{
struct ima_iint_cache *iint;
struct inode *inode = dentry->d_inode;
int must_appraise;

if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return;

must_appraise = ima_must_appraise(idmap, inode, MAY_ACCESS,
FILE_CHECK);
if (!must_appraise)
return;

/* Nothing to do if we can't allocate memory */
iint = ima_inode_get(inode);
if (!iint)
return;

/* needed for re-opening empty files */
iint->flags |= IMA_NEW_FILE;
}

>
> Thanks
>
> Roberto
>
> > For bigger changes it's also worthwhile if the object that's passed down
> > into the hook-based LSM layer is as specific as possible. If someone
> > does a change that affects lifetime rules of mounts then any hook that
> > takes a struct path argument that's unused means going through each LSM
> > that implements the hook only to find out it's not actually used.
> > Similar for dentry vs inode imho.
>

2024-03-28 11:25:10

[permalink] [raw]

Subject: Re: kernel crash in mknod

On 3/28/2024 12:08 PM, Christian Brauner wrote:
> On Thu, Mar 28, 2024 at 12:53:40PM +0200, Roberto Sassu wrote:
>> On 3/26/2024 12:40 PM, Christian Brauner wrote:
>>>> we can change the parameter of security_path_post_mknod() from
>>>> dentry to inode?
>>>
>>> If all current callers only operate on the inode then it seems the best
>>> to only pass the inode. If there's some reason someone later needs a
>>> dentry the hook can always be changed.
>>
>> Ok, so the crash is likely caused by:
>>
>> void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
>> *dentry)
>> {
>> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>>
>> I guess we can also simply check if there is an inode attached to the
>> dentry, to minimize the changes. I can do both.
>>
>> More technical question, do I need to do extra checks on the dentry before
>> calling security_path_post_mknod()?
>
> Why do you need the dentry? The two users I see are ima in [1] and evm in [2].
> Both of them don't care about the dentry. They only care about the
> inode. So why is that hook not just:

Sure, I can definitely do that. Seems an easier fix to do an extra check
in security_path_post_mknod(), rather than changing the parameter
everywhere.

Next time, when we introduce new LSM hooks we can try to introduce more
specific parameters.

Also, consider that the pre hook security_path_mknod() has the dentry as
parameter. For symmetry, we could keep it in the post hook.

What I was also asking is if I can still call d_backing_inode() on the
dentry without extra checks, and avoiding the IS_PRIVATE() check if the
former returns NULL.

> diff --git a/security/security.c b/security/security.c
> index 7e118858b545..025689a7e912 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1799,11 +1799,11 @@ EXPORT_SYMBOL(security_path_mknod);
> *
> * Update inode security field after a file has been created.
> */
> -void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
> +void security_inode_post_mknod(struct mnt_idmap *idmap, struct inode *inode)
> {
> - if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> + if (unlikely(IS_PRIVATE(inode)))
> return;
> - call_void_hook(path_post_mknod, idmap, dentry);
> + call_void_hook(path_post_mknod, idmap, inode);
> }
>
> /**
>
> And one another thing I'd like to point out is that the security hook is
> called "security_path_post_mknod()" while the evm and ima hooks are
> called evm_post_path_mknod() and ima_post_path_mknod() respectively. In
> other words:
>
> git grep _path_post_mknod() doesn't show the implementers of that hook
> which is rather unfortunate. It would be better if the pattern were:
>
> <specific LSM>_$some_$ordered_$words()

I know, yes. Didn't want to change just yet since people familiar with
the IMA code know the current function name. I don't see any problem to
rename the functions.

Thanks

Roberto

> [1]:
> static void evm_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
> {
> struct inode *inode = d_backing_inode(dentry);
> struct evm_iint_cache *iint = evm_iint_inode(inode);
>
> if (!S_ISREG(inode->i_mode))
> return;
>
> if (iint)
> iint->flags |= EVM_NEW_FILE;
> }
>
> [2]:
> static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
> {
> struct ima_iint_cache *iint;
> struct inode *inode = dentry->d_inode;
> int must_appraise;
>
> if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> return;
>
> must_appraise = ima_must_appraise(idmap, inode, MAY_ACCESS,
> FILE_CHECK);
> if (!must_appraise)
> return;
>
> /* Nothing to do if we can't allocate memory */
> iint = ima_inode_get(inode);
> if (!iint)
> return;
>
> /* needed for re-opening empty files */
> iint->flags |= IMA_NEW_FILE;
> }
>
>
>
>>
>> Thanks
>>
>> Roberto
>>
>>> For bigger changes it's also worthwhile if the object that's passed down
>>> into the hook-based LSM layer is as specific as possible. If someone
>>> does a change that affects lifetime rules of mounts then any hook that
>>> takes a struct path argument that's unused means going through each LSM
>>> that implements the hook only to find out it's not actually used.
>>> Similar for dentry vs inode imho.
>>

2024-03-28 12:09:26

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Thu, Mar 28, 2024 at 01:24:25PM +0200, Roberto Sassu wrote:
> On 3/28/2024 12:08 PM, Christian Brauner wrote:
> > On Thu, Mar 28, 2024 at 12:53:40PM +0200, Roberto Sassu wrote:
> > > On 3/26/2024 12:40 PM, Christian Brauner wrote:
> > > > > we can change the parameter of security_path_post_mknod() from
> > > > > dentry to inode?
> > > >
> > > > If all current callers only operate on the inode then it seems the best
> > > > to only pass the inode. If there's some reason someone later needs a
> > > > dentry the hook can always be changed.
> > >
> > > Ok, so the crash is likely caused by:
> > >
> > > void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
> > > *dentry)
> > > {
> > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > >
> > > I guess we can also simply check if there is an inode attached to the
> > > dentry, to minimize the changes. I can do both.
> > >
> > > More technical question, do I need to do extra checks on the dentry before
> > > calling security_path_post_mknod()?
> >
> > Why do you need the dentry? The two users I see are ima in [1] and evm in [2].
> > Both of them don't care about the dentry. They only care about the
> > inode. So why is that hook not just:
>
> Sure, I can definitely do that. Seems an easier fix to do an extra check in
> security_path_post_mknod(), rather than changing the parameter everywhere.

You only have two callers and the generic implementation.

>
> Next time, when we introduce new LSM hooks we can try to introduce more
> specific parameters.
>
> Also, consider that the pre hook security_path_mknod() has the dentry as
> parameter. For symmetry, we could keep it in the post hook.

I think that's not that important.

>
> What I was also asking is if I can still call d_backing_inode() on the
> dentry without extra checks, and avoiding the IS_PRIVATE() check if the
> former returns NULL.

2024-03-28 12:44:14

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Thu, Mar 28, 2024 at 7:24 AM Roberto Sassu
<[email protected]> wrote:
> On 3/28/2024 12:08 PM, Christian Brauner wrote:

..

> > And one another thing I'd like to point out is that the security hook is
> > called "security_path_post_mknod()" while the evm and ima hooks are
> > called evm_post_path_mknod() and ima_post_path_mknod() respectively. In
> > other words:
> >
> > git grep _path_post_mknod() doesn't show the implementers of that hook
> > which is rather unfortunate. It would be better if the pattern were:
> >
> > <specific LSM>_$some_$ordered_$words()
>
> I know, yes. Didn't want to change just yet since people familiar with
> the IMA code know the current function name. I don't see any problem to
> rename the functions.

I'm sure this is what you are planning Roberto, but just so we are all
clear on this, please do the simple bugfix to resolve the mknod
problem and then do the parameter change and the name bikeshedding in
a separate patch.

--
paul-moore.com

2024-03-28 13:04:17

[permalink] [raw]

Subject: Re: kernel crash in mknod

On Thu, Mar 28, 2024 at 8:07 AM Christian Brauner <[email protected]> wrote:
> On Thu, Mar 28, 2024 at 01:24:25PM +0200, Roberto Sassu wrote:
> > Also, consider that the pre hook security_path_mknod() has the dentry as
> > parameter. For symmetry, we could keep it in the post hook.
>
> I think that's not that important.

It is important to me. If you change security_path_post_mknod() to
take an inode, please also change security_path_mknod() to take an
inode ... actually, looking quickly at the code it looks like at least
AppArmor and TOMOYO make use of the dentry and not just the associated
inode. I didn't dive deeply into either so perhaps they could be
modified to use an inode instead, but that is a decision I would leave
up to John and Tetsuo. While Landlock does make use of the hook, it
doesn't look like it cares about anything in the dentry.

With that in mind, unless Christian has a strong argument as to why
security_path_post_mknod() must change its parameter from a dentry to
an inode, I would very much prefer to have both hooks continue to take
a dentry, unless we all decide they can be safely changed to use an
inode as a parameter. As the previous IMA/EVM hook took a dentry for
years, and Christian originally reviewed/OK'd the LSM hook, I'm
guessing there is not any significant harm in continuing to pass a
dentry, but if that isn't the case please say so ...

Of course this doesn't change anything with respect to the necessary
bugfix and/or the hook name/bikeshedding effort; no objections from me
on either.

--
paul-moore.com

2024-03-25 17:59:10