Re: [PATCH 1/2] quota: Change quotactl_path() systcall to an fd-based one

[Christian Brauner <christian.brauner@ubuntu.com>]

On Wed, Jun 02, 2021 at 05:15:52PM +0200, Jan Kara wrote:
> Some users have pointed out that path-based syscalls are problematic in
> some environments and at least directory fd argument and possibly also
> resolve flags are desirable for such syscalls. Rather than
> reimplementing all details of pathname lookup and following where it may
> eventually evolve, let's go for full file descriptor based syscall

Fair, I can accept that.

> similar to how ioctl(2) works since the beginning. Managing of quotas
> isn't performance sensitive so the extra overhead of open does not
> matter and we are able to consume O_PATH descriptors as well which makes
> open cheap anyway. Also for frequent operations (such as retrieving
> usage information for all users) we can reuse single fd and in fact get
> even better performance as well as avoiding races with possible remounts
> etc.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/quota/quota.c                  | 27 ++++++++++++---------------
>  include/linux/syscalls.h          |  4 ++--
>  include/uapi/asm-generic/unistd.h |  4 ++--
>  kernel/sys_ni.c                   |  2 +-
>  4 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 05e4bd9ab6d6..8450bb6186f4 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -968,31 +968,29 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special,
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
> -		mountpoint, qid_t, id, void __user *, addr)
> +SYSCALL_DEFINE4(quotactl_fd, unsigned int, fd, unsigned int, cmd,
> +		qid_t, id, void __user *, addr)
>  {
>  	struct super_block *sb;
> -	struct path mountpath;
>  	unsigned int cmds = cmd >> SUBCMDSHIFT;
>  	unsigned int type = cmd & SUBCMDMASK;
> +	struct fd f = fdget_raw(fd);
>  	int ret;
>  
> -	if (type >= MAXQUOTAS)
> -		return -EINVAL;
> +	if (!f.file)
> +		return -EBADF;

I would maybe change this to

f = fdget_raw(fd);
if (!f.file)
	return -EBADF;

instead of directly assigning when declaring the variable.
(And it might make sense to fold the second commit into this one.)
But other than these nits this looks good,

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  
> -	ret = user_path_at(AT_FDCWD, mountpoint,
> -			     LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
> -	if (ret)
> -		return ret;
> -
> -	sb = mountpath.mnt->mnt_sb;
> +	ret = -EINVAL;
> +	if (type >= MAXQUOTAS)
> +		goto out;
>  
>  	if (quotactl_cmd_write(cmds)) {
> -		ret = mnt_want_write(mountpath.mnt);
> +		ret = mnt_want_write(f.file->f_path.mnt);
>  		if (ret)
>  			goto out;
>  	}
>  
> +	sb = f.file->f_path.mnt->mnt_sb;
>  	if (quotactl_cmd_onoff(cmds))
>  		down_write(&sb->s_umount);
>  	else
> @@ -1006,9 +1004,8 @@ SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
>  		up_read(&sb->s_umount);
>  
>  	if (quotactl_cmd_write(cmds))
> -		mnt_drop_write(mountpath.mnt);
> +		mnt_drop_write(f.file->f_path.mnt);
>  out:
> -	path_put(&mountpath);
> -
> +	fdput(f);
>  	return ret;
>  }
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 050511e8f1f8..586128d5c3b8 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -485,8 +485,8 @@ asmlinkage long sys_pipe2(int __user *fildes, int flags);
>  /* fs/quota.c */
>  asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special,
>  				qid_t id, void __user *addr);
> -asmlinkage long sys_quotactl_path(unsigned int cmd, const char __user *mountpoint,
> -				  qid_t id, void __user *addr);
> +asmlinkage long sys_quotactl_fd(unsigned int fd, unsigned int cmd, qid_t id,
> +				void __user *addr);
>  
>  /* fs/readdir.c */
>  asmlinkage long sys_getdents64(unsigned int fd,
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 6de5a7fc066b..f211961ce1da 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -863,8 +863,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise)
>  __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2)
>  #define __NR_mount_setattr 442
>  __SYSCALL(__NR_mount_setattr, sys_mount_setattr)
> -#define __NR_quotactl_path 443
> -__SYSCALL(__NR_quotactl_path, sys_quotactl_path)
> +#define __NR_quotactl_fd 443
> +__SYSCALL(__NR_quotactl_fd, sys_quotactl_fd)
>  
>  #define __NR_landlock_create_ruleset 444
>  __SYSCALL(__NR_landlock_create_ruleset, sys_landlock_create_ruleset)
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index 0ea8128468c3..dad4d994641e 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -99,7 +99,7 @@ COND_SYSCALL(flock);
>  
>  /* fs/quota.c */
>  COND_SYSCALL(quotactl);
> -COND_SYSCALL(quotactl_path);
> +COND_SYSCALL(quotactl_fd);
>  
>  /* fs/readdir.c */
>  
> -- 
> 2.26.2
>