On Sun, May 31, 2026 at 10:56 PM NeilBrown <[email protected]> wrote: > > [Paul and Eric - question for you towards the end) > > On Mon, 01 Jun 2026, Jori Koolstra wrote: > > Calling audit_inode_child() and the i_op->create check in lookup_open() > > take place after the try_break_deleg() call. This does not match the > > order when doing a regular file create via mknod(2). There the call > > order is: > > > > filename_mknodat() > > vfs_create() > > may_create_dentry() > > audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE) > > i_op->create check > > try_break_deleg() > > > > In particular, audit_inode_child() is called regardless of whether > > try_break_deleg() fails. > > > > Move move audit_inode_child() and the i_op->create check to before > > try_break_deleg() in lookup_open().
Adding the audit mailing list to the CC line. It's almost always better to CC the list than ping folks directly as we now have the exchange in the audit archive which makes it easier for audit folks to find this in the future. > I don't think this is sufficient. > I think (and we should confirm we people who know about auditing) that > the audit call should come before the permission check so that there is > an audit record on failed attempts. In that case the > audit_inode_child() all needs to be much earlier. As you likely already know, audit_inode_child() exists simply to record information about the file/inode access, the audit record is generated later (and can is dependent on other things/config). For that reason, yes, it's generally a good idea to call audit_inode_child() as soon as we have the file/inode information. Auditors want to know what the user/system attempted to do, even if the operation failed; in some cases the log of the failed operations are more useful than the successful ones. > Alternately if we don't need an audit record for failed attempts - maybe > the audit_inode(..., AUDIT_INODE_PARENT) is enough? - then the > audit_inde_child(..., AUDIT_TYPE_CHILD_CREATE) in may_create_dentry() > should be after the permission check. > > I think you are trying to make the two paths consistent. I think that > is a good thing. I don't think you have achieved it. > > I notice that fsnotify_create() ALSO calls audit_inode_child(... > AUDIT_TYPE_CHILD_CREATE). Surely we don't need both? > > Paul, Eric: can you comment on whether AUDIT_TYPE_CHILD_CREATE() should > come before permission checks, after checks but before create, or after > the create? Or maybe several of those? > We seem to do it both before in may_create_dentry(), and after in > fsnotify_create(). > > > Thanks, > NeilBrown > > > Signed-off-by: Jori Koolstra <[email protected]> > > --- > > fs/namei.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 16dcf537db87..293592a4d011 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -4526,17 +4526,18 @@ static struct dentry *lookup_open(struct nameidata > > *nd, struct file *file, > > > > /* Negative dentry, just create the file */ > > if (!dentry->d_inode && (open_flag & O_CREAT)) { > > - /* but break the directory lease first! */ > > - error = try_break_deleg(dir_inode, delegated_inode); > > - if (error) > > - goto out_dput; > > - > > audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE); > > + > > if (!dir_inode->i_op->create) { > > error = -EACCES; > > goto out_dput; > > } > > > > + /* but break the directory lease first! */ > > + error = try_break_deleg(dir_inode, delegated_inode); > > + if (error) > > + goto out_dput; > > + > > error = dir_inode->i_op->create(idmap, dir_inode, dentry, > > mode, open_flag & O_EXCL); > > if (error) > > -- > > 2.54.0 -- paul-moore.com
