Re: [PATCH v3 01/13] filelock: push the S_ISREG check down to ->setlease handlers

On Tue 21-10-25 11:25:36, Jeff Layton wrote:
> When nfsd starts requesting directory delegations, setlease handlers may
> see requests for leases on directories. Push the !S_ISREG check down
> into the non-trivial setlease handlers, so we can selectively enable
> them where they're supported.
> 
> FUSE is special: It's the only filesystem that supports atomic_open and
> allows kernel-internal leases. atomic_open is issued when the VFS
> doesn't know the state of the dentry being opened. If the file doesn't
> exist, it may be created, in which case the dir lease should be broken.
> 
> The existing kernel-internal lease implementation has no provision for
> this. Ensure that we don't allow directory leases by default going
> forward by explicitly disabling them there.
> 
> Reviewed-by: NeilBrown <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/fuse/dir.c          | 1 +
>  fs/locks.c             | 5 +++--
>  fs/nfs/nfs4file.c      | 2 ++
>  fs/smb/client/cifsfs.c | 3 +++
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index ecaec0fea3a132e7cbb88121e7db7fb504d57d3c..667774cc72a1d49796f531fcb342d2e4878beb85 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -2230,6 +2230,7 @@ static const struct file_operations fuse_dir_operations = {
>  	.fsync		= fuse_dir_fsync,
>  	.unlocked_ioctl	= fuse_dir_ioctl,
>  	.compat_ioctl	= fuse_dir_compat_ioctl,
> +	.setlease	= simple_nosetlease,
>  };
>  
>  static const struct inode_operations fuse_common_inode_operations = {
> diff --git a/fs/locks.c b/fs/locks.c
> index 04a3f0e2072461b6e2d3d1cd12f2b089d69a7db3..0b16921fb52e602ea2e0c3de39d9d772af98ba7d 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1929,6 +1929,9 @@ static int generic_delete_lease(struct file *filp, void *owner)
>  int generic_setlease(struct file *filp, int arg, struct file_lease **flp,
>  			void **priv)
>  {
> +	if (!S_ISREG(file_inode(filp)->i_mode))
> +		return -EINVAL;
> +
>  	switch (arg) {
>  	case F_UNLCK:
>  		return generic_delete_lease(filp, *priv);
> @@ -2018,8 +2021,6 @@ vfs_setlease(struct file *filp, int arg, struct file_lease **lease, void **priv)
>  
>  	if ((!vfsuid_eq_kuid(vfsuid, current_fsuid())) && !capable(CAP_LEASE))
>  		return -EACCES;
> -	if (!S_ISREG(inode->i_mode))
> -		return -EINVAL;
>  	error = security_file_lock(filp, arg);
>  	if (error)
>  		return error;
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index 7f43e890d3564a000dab9365048a3e17dc96395c..7317f26892c5782a39660cae87ec1afea24e36c0 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -431,6 +431,8 @@ void nfs42_ssc_unregister_ops(void)
>  static int nfs4_setlease(struct file *file, int arg, struct file_lease **lease,
>  			 void **priv)
>  {
> +	if (!S_ISREG(file_inode(file)->i_mode))
> +		return -EINVAL;
>  	return nfs4_proc_setlease(file, arg, lease, priv);
>  }
>  
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 4f959f1e08d235071a151c1438c753fcd05099e5..1522c6b61b48c05c93f2bedeab0d35b6d85378e2 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1149,6 +1149,9 @@ cifs_setlease(struct file *file, int arg, struct file_lease **lease, void **priv
>  	struct inode *inode = file_inode(file);
>  	struct cifsFileInfo *cfile = file->private_data;
>  
> +	if (!S_ISREG(inode->i_mode))
> +		return -EINVAL;
> +
>  	/* Check if file is oplocked if this is request for new lease */
>  	if (arg == F_UNLCK ||
>  	    ((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) ||
> 
> -- 
> 2.51.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR