On Thu 03-06-21 14:25:04, Christian Brauner wrote:
> 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@xxxxxxx>
> > ---
> > 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.
OK, I'll do this.
> (And it might make sense to fold the second commit into this one.)
Well, I wanted to keep mostly mechanical (and partly autogenerated ;))
changes separately from the "real" stuff that needs serious review... So I
prefer to keep the split.
> But other than these nits this looks good,
>
> Acked-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
Thanks for review!
Honza
>
> >
> > - 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
> >
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
