Re: [RFC PATCH v6 3/9] vfs: prepare vfs_creat|mkdir_no_perm for reuse in lookup_open()

[NeilBrown]

On Mon, 01 Jun 2026, Jori Koolstra wrote:
> To implement O_CREAT|O_DIRECTORY we will have to repeat some of the
> logic that is now in vfs_mkdir() (e.g. do error checks in the same
> order). Separate this out in vfs_mkdir_no_perm(), which does all the
> non-permission related work of vfs_mkdir(). Permission checking for the
> lookup_open() path is timed differently because we may just be doing an
> open and no create. Similar considerations give rise to
> vfs_create_no_perm().
> 
> Moving the fsnotify_* calls also allows us to deal with this in one
> place for each type of operation. This does mean that we also need
> to move the fsnotify_* calls into atomic_open() for the atomic open
> case, but this actually reduces duplicate code in open_last_lookups()
> and dentry_create().
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
> ---
>  fs/namei.c | 105 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 68 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4cd132fe3981..2a35dd72ee96 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4171,6 +4171,24 @@ static inline umode_t vfs_prepare_mode(struct mnt_idmap *idmap,
>  	return mode;
>  }
>  
> +static inline
> +int vfs_create_no_perm(struct mnt_idmap *idmap, struct dentry *dentry,
> +		       umode_t mode, struct delegated_inode *di, bool excl)
> +{
> +	struct inode *dir = d_inode(dentry->d_parent);
> +	int error;
> +
> +	error = try_break_deleg(dir, di);
> +	if (error)
> +		return error;
> +
> +	error = dir->i_op->create(idmap, dir, dentry, mode, excl);
> +	if (!error)
> +		fsnotify_create(dir, dentry);
> +
> +	return error;
> +}
> +
>  /**
>   * vfs_create - create new file
>   * @idmap:	idmap of the mount the inode was found from
> @@ -4203,13 +4221,8 @@ int vfs_create(struct mnt_idmap *idmap, struct dentry *dentry, umode_t mode,
>  	error = security_inode_create(dir, dentry, mode);
>  	if (error)
>  		return error;
> -	error = try_break_deleg(dir, di);
> -	if (error)
> -		return error;
> -	error = dir->i_op->create(idmap, dir, dentry, mode, true);
> -	if (!error)
> -		fsnotify_create(dir, dentry);
> -	return error;
> +
> +	return vfs_create_no_perm(idmap, dentry, mode, di, /*excl=*/ true);
>  }
>  EXPORT_SYMBOL(vfs_create);
>  
> @@ -4386,10 +4399,17 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
>  				error = -ENOENT;
>  		}
>  	}
> +
>  	if (error) {
>  		dput(dentry);
>  		dentry = ERR_PTR(error);
> +	} else {
> +		if (file->f_mode & FMODE_CREATED)
> +			fsnotify_create(dir, dentry);
> +		if (file->f_mode & FMODE_OPENED)
> +			fsnotify_open(file);
>  	}
> +
>  	return dentry;
>  }
>  
> @@ -4522,6 +4542,8 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  						mode, open_flag & O_EXCL);
>  		if (error)
>  			goto out_dput;
> +
> +		fsnotify_create(dir_inode, dentry);
>  	}
>  
>  	return dentry;
> @@ -4610,13 +4632,9 @@ static const char *open_last_lookups(struct nameidata *nd,
>  		inode_lock(dir->d_inode);
>  	else
>  		inode_lock_shared(dir->d_inode);
> +
>  	dentry = lookup_open(nd, file, op, got_write, &delegated_inode);
> -	if (!IS_ERR(dentry)) {
> -		if (file->f_mode & FMODE_CREATED)
> -			fsnotify_create(dir->d_inode, dentry);
> -		if (file->f_mode & FMODE_OPENED)
> -			fsnotify_open(file);
> -	}
> +
>  	if (open_flag & O_CREAT)
>  		inode_unlock(dir->d_inode);
>  	else
> @@ -5051,13 +5069,6 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  		if (unlikely(create_error) && error == -ENOENT)
>  			error = create_error;
>  
> -		if (!error) {
> -			if (file->f_mode & FMODE_CREATED)
> -				fsnotify_create(dir->d_inode, dentry);
> -			if (file->f_mode & FMODE_OPENED)
> -				fsnotify_open(file);
> -		}
> -
>  		path->dentry = dentry;
>  
>  	} else {
> @@ -5209,6 +5220,39 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d
>  	return filename_mknodat(AT_FDCWD, name, mode, dev);
>  }
>  
> +static inline
> +struct dentry *vfs_mkdir_no_perm(struct mnt_idmap *idmap, struct inode *dir,
> +				 struct dentry *dentry, umode_t mode,
> +				 struct delegated_inode *di)
> +{
> +	int error;
> +	struct dentry *de;
> +	unsigned max_links = dir->i_sb->s_max_links;
> +
> +	error = -EMLINK;
> +	if (max_links && dir->i_nlink >= max_links)
> +		goto err;
> +
> +	error = try_break_deleg(dir, di);
> +	if (error)
> +		goto err;
> +
> +	de = dir->i_op->mkdir(idmap, dir, dentry, mode);
> +	if (IS_ERR(de)) {
> +		error = PTR_ERR(de);
> +		goto err;
> +	}
> +	if (de) {
> +		dput(dentry);
> +		dentry = de;
> +	}
> +	fsnotify_mkdir(dir, dentry);
> +	return dentry;
> +
> +err:
> +	return ERR_PTR(error);
> +}
> +
>  /**
>   * vfs_mkdir - create directory returning correct dentry if possible
>   * @idmap:		idmap of the mount the inode was found from
> @@ -5236,7 +5280,6 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  			 struct delegated_inode *delegated_inode)
>  {
>  	int error;
> -	unsigned max_links = dir->i_sb->s_max_links;
>  	struct dentry *de;
>  
>  	error = may_create_dentry(idmap, dir, dentry);
> @@ -5252,24 +5295,12 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	if (error)
>  		goto err;
>  
> -	error = -EMLINK;
> -	if (max_links && dir->i_nlink >= max_links)
> +	de = vfs_mkdir_no_perm(idmap, dir, dentry, mode, delegated_inode);
> +	if (IS_ERR(de)) {
> +		error = PTR_ERR(de);
>  		goto err;
> -
> -	error = try_break_deleg(dir, delegated_inode);
> -	if (error)
> -		goto err;
> -
> -	de = dir->i_op->mkdir(idmap, dir, dentry, mode);
> -	error = PTR_ERR(de);
> -	if (IS_ERR(de))
> -		goto err;
> -	if (de) {
> -		dput(dentry);
> -		dentry = de;
>  	}
> -	fsnotify_mkdir(dir, dentry);
> -	return dentry;
> +	return de;
>  
>  err:
>  	end_creating(dentry);
> -- 
> 2.54.0
> 
> 

This looks good.  I definitely like the idea of moving fsnotify_* calls
closer to where the action happens.

Reviewed-by: NeilBrown <neil@xxxxxxxxxx>

Thanks,
NeilBrown