From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 02C75C433B4 for ; Thu, 15 Apr 2021 10:09:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DAE9161249 for ; Thu, 15 Apr 2021 10:09:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232113AbhDOKJ7 (ORCPT ); Thu, 15 Apr 2021 06:09:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:40636 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231482AbhDOKJ7 (ORCPT ); Thu, 15 Apr 2021 06:09:59 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id CAD5B6109F; Thu, 15 Apr 2021 10:09:33 +0000 (UTC) Date: Thu, 15 Apr 2021 12:09:28 +0200 From: Christian Brauner To: Dmitry Kadashev Cc: Jens Axboe , Alexander Viro , Pavel Begunkov , Christoph Hellwig , linux-fsdevel@vger.kernel.org, io-uring Subject: Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename Message-ID: <20210415100928.3ukgiaui4rhspiq6@wittgenstein> References: <20210330055957.3684579-1-dkadashev@gmail.com> <20210330055957.3684579-2-dkadashev@gmail.com> <20210330071700.kpjoyp5zlni7uejm@wittgenstein> <20210415100815.edrn4a7cy26wkowe@wittgenstein> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210415100815.edrn4a7cy26wkowe@wittgenstein> Precedence: bulk List-ID: X-Mailing-List: io-uring@vger.kernel.org On Thu, Apr 15, 2021 at 12:08:20PM +0200, Christian Brauner wrote: > On Thu, Apr 15, 2021 at 02:14:17PM +0700, Dmitry Kadashev wrote: > > On Thu, Apr 8, 2021 at 3:45 PM Dmitry Kadashev wrote: > > > > > > On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner > > > wrote: > > > > The only thing that is a bit unpleasant here is that this change > > > > breaks the consistency between the creation helpers: > > > > > > > > do_mkdirat() > > > > do_symlinkat() > > > > do_linkat() > > > > do_mknodat() > > > > > > > > All but of them currently take > > > > const char __user *pathname > > > > and call > > > > user_path_create() > > > > with that do_mkdirat() change that's no longer true. One of the major > > > > benefits over the recent years in this code is naming and type consistency. > > > > And since it's just matter of time until io_uring will also gain support > > > > for do_{symlinkat,linkat,mknodat} I would think switching all of them to > > > > take a struct filename > > > > and then have all do_* helpers call getname() might just be nicer in the > > > > long run. > > > > > > So, I've finally got some time to look into this. do_mknodat() and > > > do_symlinkat() are easy. But do_linkat() is more complicated, I could use some > > > hints as to what's the reasonable way to implement the change. > > > > > > The problem is linkat() requires CAP_DAC_READ_SEARCH capability if AT_EMPTY_PATH > > > flag is passed. Right now do_linkat checks the capability before calling > > > getname_flags (essentially). If do_linkat is changed to accept struct filename > > > then there is no bulletproof way to force CAP_DAC_READ_SEARCH presence (e.g. if > > > for whatever reason AT_EMPTY_PATH is not in flags passed to do_linkat). Also, it > > > means that the caller is responsible to process AT_EMPTY_PATH in the first > > > place, which means logic duplication. > > > > > > Any ideas what's the best way to approach this? > > > > Ping. If someone can see how we can avoid making do_linkat() callers > > ensure the process has CAP_DAC_READ_SEARCH capability if AT_EMPTY_PATH > > was passed then the hints would be really appreciated. > > Would something like this help? Because I always forget this: _Completely_ untested. (And sorry for the late reply. Just a lot of mail.) > > From 7adeec2fe4a954e4e4b8a158a4d9fe705b82b978 Mon Sep 17 00:00:00 2001 > From: Christian Brauner > Date: Thu, 15 Apr 2021 12:03:42 +0200 > Subject: [PATCH] namei: add getname_uflags() > > There are a couple of places where we already open-code the (flags & > AT_EMPTY_PATH) check and io_uring will likely add another one in the future. > Let's just add a simple helper getname_uflags() that handles this directly and > use it. > getname_flags() itself doesn't need access to lookup flags other than > LOOKUP_EMPTY so this is basically just a boolean already so be honest about it. > > Signed-off-by: Christian Brauner > --- > fs/exec.c | 10 ++-------- > fs/fsopen.c | 6 +++--- > fs/namei.c | 6 +++--- > include/linux/fs.h | 4 ++++ > 4 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 18594f11c31f..53c633f69f4a 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -2069,10 +2069,7 @@ SYSCALL_DEFINE5(execveat, > const char __user *const __user *, envp, > int, flags) > { > - int lookup_flags = (flags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0; > - > - return do_execveat(fd, > - getname_flags(filename, lookup_flags, NULL), > + return do_execveat(fd, getname_uflags(filename, flags), > argv, envp, flags); > } > > @@ -2090,10 +2087,7 @@ COMPAT_SYSCALL_DEFINE5(execveat, int, fd, > const compat_uptr_t __user *, envp, > int, flags) > { > - int lookup_flags = (flags & AT_EMPTY_PATH) ? LOOKUP_EMPTY : 0; > - > - return compat_do_execveat(fd, > - getname_flags(filename, lookup_flags, NULL), > + return compat_do_execveat(fd, getname_uflags(filename, flags), > argv, envp, flags); > } > #endif > diff --git a/fs/fsopen.c b/fs/fsopen.c > index 27a890aa493a..00906abaf466 100644 > --- a/fs/fsopen.c > +++ b/fs/fsopen.c > @@ -321,7 +321,7 @@ SYSCALL_DEFINE5(fsconfig, > struct fs_context *fc; > struct fd f; > int ret; > - int lookup_flags = 0; > + bool lookup_empty = false; > > struct fs_parameter param = { > .type = fs_value_is_undefined, > @@ -411,11 +411,11 @@ SYSCALL_DEFINE5(fsconfig, > } > break; > case FSCONFIG_SET_PATH_EMPTY: > - lookup_flags = LOOKUP_EMPTY; > + lookup_empty = true; > fallthrough; > case FSCONFIG_SET_PATH: > param.type = fs_value_is_filename; > - param.name = getname_flags(_value, lookup_flags, NULL); > + param.name = getname_flags(_value, lookup_empty, NULL); > if (IS_ERR(param.name)) { > ret = PTR_ERR(param.name); > goto out_key; > diff --git a/fs/namei.c b/fs/namei.c > index 216f16e74351..7694f6bcd711 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -125,7 +125,7 @@ > #define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname)) > > struct filename * > -getname_flags(const char __user *filename, int flags, int *empty) > +getname_flags(const char __user *filename, bool lookup_empty, int *empty) > { > struct filename *result; > char *kname; > @@ -191,7 +191,7 @@ getname_flags(const char __user *filename, int flags, int *empty) > if (unlikely(!len)) { > if (empty) > *empty = 1; > - if (!(flags & LOOKUP_EMPTY)) { > + if (lookup_empty) { > putname(result); > return ERR_PTR(-ENOENT); > } > @@ -206,7 +206,7 @@ getname_flags(const char __user *filename, int flags, int *empty) > struct filename * > getname(const char __user * filename) > { > - return getname_flags(filename, 0, NULL); > + return getname_flags(filename, false, NULL); > } > > struct filename * > diff --git a/include/linux/fs.h b/include/linux/fs.h > index ec8f3ddf4a6a..6dbd629ece04 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2644,6 +2644,10 @@ static inline struct file *file_clone_open(struct file *file) > extern int filp_close(struct file *, fl_owner_t id); > > extern struct filename *getname_flags(const char __user *, int, int *); > +extern struct filename *getname_uflags(const char __user *filename, int uflags) > +{ > + return getname_flags(filename, (flags & AT_EMPTY_PATH), NULL); > +} > extern struct filename *getname(const char __user *); > extern struct filename *getname_kernel(const char *); > extern void putname(struct filename *name); > -- > 2.27.0 >