Re: [PATCH v3 03/14] vfs: prepare vfs_creat|mkdir_no_perm for reuse in lookup_open()

[Christian Brauner]

> 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().
> 
> Reviewed-by: NeilBrown <neil@xxxxxxxxxx>
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 40f83a942e12..ee47d3f5159f 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4169,6 +4169,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)
> +{
> +	struct inode *dir = d_inode(dentry->d_parent);
> +	int error;
> +
> +	error = try_break_deleg(dir, LEASE_BREAK_DIR_CREATE, di);
> +	if (error)
> +		return error;
> +
> +	error = dir->i_op->create(idmap, dir, dentry, mode, true);
> +	if (!error)
> +		fsnotify_create(dir, dentry);
> +
> +	return error;
> +}
> +
>  /**
>   * vfs_create - create new file
>   * @idmap:	idmap of the mount the inode was found from
> @@ -4201,13 +4219,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, LEASE_BREAK_DIR_CREATE, 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);
>  }
>  EXPORT_SYMBOL(vfs_create);
>  
> @@ -4384,10 +4397,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;
>  }
>  
> @@ -4519,6 +4539,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;
> @@ -4607,13 +4629,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
> @@ -5066,13 +5084,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 {


> @@ -5224,6 +5235,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;

All the gotos here seem unnecessary. You can just return directly as
there's no cleanup performed under the err: label.

-- 
Christian Brauner <brauner@xxxxxxxxxx>