Re: [PATCH v3 04/14] vfs: call audit_inode_child() in lookup_open() on failure too

[NeilBrown]

On Tue, 07 Jul 2026, Jori Koolstra wrote:
> > Op 06-07-2026 07:33 CEST schreef NeilBrown <neilb@xxxxxxxxxxx>:
> > 
> >  
> > On Sun, 05 Jul 2026, Jori Koolstra wrote:
> > > audit_inode_child() is called in may_create_dentry() so that failed
> > > filesystem operations still register an audit entry. On success, the
> > > entry is overwritten when, for instance, fsnotify_create() is called.
> > > This is the calling convention in vfs_create() and vfs_mkdir().
> > > In lookup_open(), however, when atomic_open() should have created a
> > > file but didn't, no call to audit_inode_child() is made. The same is
> > > true for the regular ->create() path.
> > > 
> > > Fix the calling of audit_inode_child() in lookup_open() to match the
> > > vfs_create() path. For the ->atomic_open() filesystems this logic has
> > > been pushed into atomic_open(). This function is also reordered a bit
> > > to make the case distinction of the possible returns from
> > > ->atomic_open() more explicit (i.e. finish_open() or finish_no_open()).
> > > 
> > > When retrying delegation breaking, audit_inode_child() could be called
> > > more than once, but this is OK because those entries are reused.
> > > 
> > > Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
> > > ---
> > >  fs/namei.c | 75 ++++++++++++++++++++++++++++++++----------------------
> > >  1 file changed, 45 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index ee47d3f5159f..956adcd14c4a 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -4369,10 +4369,11 @@ static int may_o_create(struct mnt_idmap *idmap,
> > >   */
> > >  static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
> > >  				  struct file *file,
> > > -				  int open_flag, umode_t mode)
> > > +				  int open_flag, umode_t mode, bool create_error)
> > >  {
> > >  	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
> > >  	struct inode *dir =  path->dentry->d_inode;
> > > +	bool failed_create = false;
> > >  	int error;
> > >  
> > >  	file->__f_path.dentry = DENTRY_NOT_SET;
> > > @@ -4382,23 +4383,33 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
> > >  	d_lookup_done(dentry);
> > >  	if (!error) {
> > >  		if (file->f_mode & FMODE_OPENED) {
> > > -			if (unlikely(dentry != file->f_path.dentry)) {
> > > +			// finish_open() called
> > > +			struct dentry *opened = file->f_path.dentry;
> > > +			if (unlikely(opened != dentry)) {
> > >  				dput(dentry);
> > > -				dentry = dget(file->f_path.dentry);
> > > +				dentry = dget(opened);
> > >  			}
> > > -		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
> > > -			error = -EIO;
> > > -		} else {
> > > -			if (file->f_path.dentry) {
> > > +		} else if (likely(file->f_path.dentry != DENTRY_NOT_SET)) {
> > > +			// finish_no_open() called
> > > +			struct dentry *replaced = file->f_path.dentry;
> > > +			if (replaced) {
> > >  				dput(dentry);
> > > -				dentry = file->f_path.dentry;
> > > +				dentry = replaced;
> > >  			}
> > >  			if (unlikely(d_is_negative(dentry)))
> > >  				error = -ENOENT;
> > 
> > I think this can now be
> >    if (unlikely(d_is_negative(dentry))
> >          error = create_error ?: -ENOENT;
> > or something similar if people don't like "?:".
> > 
> 
> create_error is a bool, and the caller still expects -ENOENT because we have

Oh, is it?  So it is, I missed that.  I would much rather it was an int.

> 
> 		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
> 		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
> 			dentry = ERR_PTR(create_error);

You wouldn't need this 'if" any more if you made the change I suggested.

> 
> > > +			if (error && create_error) // should have created, but errored before
> > > +				failed_create = true;
> > 
> > Why have 'failed_create' ? Why not just put the audit_inode_child() like
> > here?
> > 
> 
> Because I am a doofus and didn't notice during several rewrites.

Happens to everyone :-)

> 
> If we reach the point where you propose we move audit_inode_child() to, then error
> is true and so we reach audit_inode_child(). Conversely, if we reached
> audit_inode_child() then failed_create is true, and this is only set at the proposed
> place to move the function to. So this is indeed equivalent.
> 
> The failed_create name is terrible anyway and it tripped Sashiko over. It was meant
> to convey rather "we know for sure that we should have created a file."
> 
> Thanks a lot for taking the time to review and the helpful feedback!

A pleasure.

NeilBrown

> 
> Best,
> Jori.
> 
> > There rest of the patch look pretty good.
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > > +		} else {
> > > +			const char *fsname = dentry->d_sb->s_type->name;
> > > +			WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n", fsname);
> > > +			error = -EIO;
> > >  		}
> > >  	}
> > >  
> > >  	if (error) {
> > > +		if (failed_create)
> > > +			audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
> > >  		dput(dentry);
> > >  		dentry = ERR_PTR(error);
> > >  	} else {
> > > @@ -4462,7 +4473,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > >  		dentry = NULL;
> > >  	}
> > >  	if (dentry->d_inode) {
> > > -		/* Cached positive dentry: will open in f_op->open */
> > > +		/* Cached positive dentry: will open in do_open(). */
> > >  		return dentry;
> > >  	}
> > >  
> > > @@ -4496,7 +4507,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > >  	if (dir_inode->i_op->atomic_open) {
> > >  		if (nd->flags & LOOKUP_DIRECTORY)
> > >  			open_flag |= O_DIRECTORY;
> > > -		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
> > > +		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode, create_error);
> > >  		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
> > >  			dentry = ERR_PTR(create_error);
> > >  		return dentry;
> > > @@ -4515,33 +4526,37 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > >  			dentry = res;
> > >  		}
> > >  	}
> > > +	if (dentry->d_inode || !(op->open_flag & O_CREAT)) {
> > > +		/* No need to create a file. If lookup returned a positive
> > > +		 * dentry, the file will be opened in do_open(). */
> > > +		return dentry;
> > > +	}
> > > +
> > > +	/* Negative dentry with O_CREAT flag set */
> > > +	audit_inode_child(dir->d_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> > >  
> > > -	if (unlikely(create_error) && !dentry->d_inode) {
> > > +	if (unlikely(create_error)) {
> > > +		/* Should have done a create, but we already errored */
> > >  		error = create_error;
> > >  		goto out_dput;
> > >  	}
> > >  
> > > -	/* 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, LEASE_BREAK_DIR_CREATE, delegated_inode);
> > > -		if (error)
> > > -			goto out_dput;
> > > +	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> > > +	if (error)
> > > +		goto out_dput;
> > >  
> > > -		file->f_mode |= FMODE_CREATED;
> > > -		audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> > > -		if (!dir_inode->i_op->create) {
> > > -			error = -EACCES;
> > > -			goto out_dput;
> > > -		}
> > > +	file->f_mode |= FMODE_CREATED;
> > > +	if (!dir_inode->i_op->create) {
> > > +		error = -EACCES;
> > > +		goto out_dput;
> > > +	}
> > >  
> > > -		error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> > > -						mode, open_flag & O_EXCL);
> > > -		if (error)
> > > -			goto out_dput;
> > > +	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> > > +					mode, open_flag & O_EXCL);
> > > +	if (error)
> > > +		goto out_dput;
> > >  
> > > -		fsnotify_create(dir_inode, dentry);
> > > -	}
> > > +	fsnotify_create(dir_inode, dentry);
> > >  
> > >  	return dentry;
> > >  
> > > @@ -5071,7 +5086,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
> > >  
> > >  		/* atomic_open will dput(dentry) on error */
> > >  		dget(orig_dentry);
> > > -		dentry = atomic_open(path, dentry, file, flags, mode);
> > > +		dentry = atomic_open(path, dentry, file, flags, mode, create_error);
> > >  		error = PTR_ERR_OR_ZERO(dentry);
> > >  
> > >  		if (IS_ERR(dentry))
> > > -- 
> > > 2.55.0
> > > 
> > >
>