Re: [RFC PATCH v6 5/9] vfs: lookup_open(): move audit_inode_child() and i_op->create check to before try_break_deleg()

[NeilBrown]

[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().

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.

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 <jkoolstra@xxxxxxxxx>
> ---
>  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
> 
>