[PATCH v3 0/2] io_uring: add mkdirat support

[PATCH v3 0/2] io_uring: add mkdirat support

[Dmitry Kadashev]

This adds mkdirat support to io_uring and is heavily based on recently
added renameat() / unlinkat() support.

The first patch is preparation with no functional changes, makes
do_mkdirat accept struct filename pointer rather than the user string.

The second one leverages that to implement mkdirat in io_uring.

Based on for-5.13/io_uring.

Changes since v2:
- rebase

Changes since v1:
- do not mess with struct filename's refcount in do_mkdirat, instead add
  and use __filename_create() that does not drop the name on success;

Dmitry Kadashev (2):
  fs: make do_mkdirat() take struct filename
  io_uring: add support for IORING_OP_MKDIRAT

 fs/internal.h                 |  1 +
 fs/io_uring.c                 | 55 +++++++++++++++++++++++++++++++++++
 fs/namei.c                    | 25 ++++++++++++----
 include/uapi/linux/io_uring.h |  1 +
 4 files changed, 76 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

Pass in the struct filename pointers instead of the user string, and
update the three callers to do the same. This is heavily based on
commit dbea8d345177 ("fs: make do_renameat2() take struct filename").

This behaves like do_unlinkat() and do_renameat2().

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/internal.h |  1 +
 fs/namei.c    | 25 +++++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 6aeae7ef3380..848e165ef0f1 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -77,6 +77,7 @@ long do_unlinkat(int dfd, struct filename *name);
 int may_linkat(struct user_namespace *mnt_userns, struct path *link);
 int do_renameat2(int olddfd, struct filename *oldname, int newdfd,
 		 struct filename *newname, unsigned int flags);
+long do_mkdirat(int dfd, struct filename *name, umode_t mode);
 
 /*
  * namespace.c
diff --git a/fs/namei.c b/fs/namei.c
index 216f16e74351..bfdc3128bf8d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3560,7 +3560,7 @@ struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
 	return file;
 }
 
-static struct dentry *filename_create(int dfd, struct filename *name,
+static struct dentry *__filename_create(int dfd, struct filename *name,
 				struct path *path, unsigned int lookup_flags)
 {
 	struct dentry *dentry = ERR_PTR(-EEXIST);
@@ -3616,7 +3616,6 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 		error = err2;
 		goto fail;
 	}
-	putname(name);
 	return dentry;
 fail:
 	dput(dentry);
@@ -3631,6 +3630,16 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 	return dentry;
 }
 
+static inline struct dentry *filename_create(int dfd, struct filename *name,
+				struct path *path, unsigned int lookup_flags)
+{
+	struct dentry *res = __filename_create(dfd, name, path, lookup_flags);
+
+	if (!IS_ERR(res))
+		putname(name);
+	return res;
+}
+
 struct dentry *kern_path_create(int dfd, const char *pathname,
 				struct path *path, unsigned int lookup_flags)
 {
@@ -3821,15 +3830,18 @@ int vfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 }
 EXPORT_SYMBOL(vfs_mkdir);
 
-static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
+long do_mkdirat(int dfd, struct filename *name, umode_t mode)
 {
 	struct dentry *dentry;
 	struct path path;
 	int error;
 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
 
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
 retry:
-	dentry = user_path_create(dfd, pathname, &path, lookup_flags);
+	dentry = __filename_create(dfd, name, &path, lookup_flags);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
@@ -3847,17 +3859,18 @@ static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
 		lookup_flags |= LOOKUP_REVAL;
 		goto retry;
 	}
+	putname(name);
 	return error;
 }
 
 SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
 {
-	return do_mkdirat(dfd, pathname, mode);
+	return do_mkdirat(dfd, getname(pathname), mode);
 }
 
 SYSCALL_DEFINE2(mkdir, const char __user *, pathname, umode_t, mode)
 {
-	return do_mkdirat(AT_FDCWD, pathname, mode);
+	return do_mkdirat(AT_FDCWD, getname(pathname), mode);
 }
 
 /**
-- 
2.30.2

[PATCH v3 2/2] io_uring: add support for IORING_OP_MKDIRAT

[Dmitry Kadashev]

IORING_OP_MKDIRAT behaves like mkdirat(2) and takes the same flags
and arguments.

Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/io_uring.c                 | 55 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 56 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9a1a02fb3c9a..d9c100ed6132 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -663,6 +663,13 @@ struct io_unlink {
 	struct filename			*filename;
 };
 
+struct io_mkdir {
+	struct file			*file;
+	int				dfd;
+	umode_t				mode;
+	struct filename			*filename;
+};
+
 struct io_completion {
 	struct file			*file;
 	struct list_head		list;
@@ -803,6 +810,7 @@ struct io_kiocb {
 		struct io_shutdown	shutdown;
 		struct io_rename	rename;
 		struct io_unlink	unlink;
+		struct io_mkdir		mkdir;
 		/* use only after cleaning per-op data, see io_clean_op() */
 		struct io_completion	compl;
 	};
@@ -1016,6 +1024,7 @@ static const struct io_op_def io_op_defs[] = {
 	},
 	[IORING_OP_RENAMEAT] = {},
 	[IORING_OP_UNLINKAT] = {},
+	[IORING_OP_MKDIRAT] = {},
 };
 
 static bool io_disarm_next(struct io_kiocb *req);
@@ -3523,6 +3532,44 @@ static int io_unlinkat(struct io_kiocb *req, unsigned int issue_flags)
 	return 0;
 }
 
+static int io_mkdirat_prep(struct io_kiocb *req,
+			    const struct io_uring_sqe *sqe)
+{
+	struct io_mkdir *mkd = &req->mkdir;
+	const char __user *fname;
+
+	if (unlikely(req->flags & REQ_F_FIXED_FILE))
+		return -EBADF;
+
+	mkd->dfd = READ_ONCE(sqe->fd);
+	mkd->mode = READ_ONCE(sqe->len);
+
+	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
+	mkd->filename = getname(fname);
+	if (IS_ERR(mkd->filename))
+		return PTR_ERR(mkd->filename);
+
+	req->flags |= REQ_F_NEED_CLEANUP;
+	return 0;
+}
+
+static int io_mkdirat(struct io_kiocb *req, int issue_flags)
+{
+	struct io_mkdir *mkd = &req->mkdir;
+	int ret;
+
+	if (issue_flags & IO_URING_F_NONBLOCK)
+		return -EAGAIN;
+
+	ret = do_mkdirat(mkd->dfd, mkd->filename, mkd->mode);
+
+	req->flags &= ~REQ_F_NEED_CLEANUP;
+	if (ret < 0)
+		req_set_fail_links(req);
+	io_req_complete(req, ret);
+	return 0;
+}
+
 static int io_shutdown_prep(struct io_kiocb *req,
 			    const struct io_uring_sqe *sqe)
 {
@@ -5942,6 +5989,8 @@ static int io_req_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return io_renameat_prep(req, sqe);
 	case IORING_OP_UNLINKAT:
 		return io_unlinkat_prep(req, sqe);
+	case IORING_OP_MKDIRAT:
+		return io_mkdirat_prep(req, sqe);
 	}
 
 	printk_once(KERN_WARNING "io_uring: unhandled opcode %d\n",
@@ -6083,6 +6132,9 @@ static void io_clean_op(struct io_kiocb *req)
 		case IORING_OP_UNLINKAT:
 			putname(req->unlink.filename);
 			break;
+		case IORING_OP_MKDIRAT:
+			putname(req->mkdir.filename);
+			break;
 		}
 		req->flags &= ~REQ_F_NEED_CLEANUP;
 	}
@@ -6198,6 +6250,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
 	case IORING_OP_UNLINKAT:
 		ret = io_unlinkat(req, issue_flags);
 		break;
+	case IORING_OP_MKDIRAT:
+		ret = io_mkdirat(req, issue_flags);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 5beaa6bbc6db..cf26a94ab880 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -137,6 +137,7 @@ enum {
 	IORING_OP_SHUTDOWN,
 	IORING_OP_RENAMEAT,
 	IORING_OP_UNLINKAT,
+	IORING_OP_MKDIRAT,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,
-- 
2.30.2

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner]

On Tue, Mar 30, 2021 at 12:59:56PM +0700, Dmitry Kadashev wrote:
> Pass in the struct filename pointers instead of the user string, and
> update the three callers to do the same. This is heavily based on
> commit dbea8d345177 ("fs: make do_renameat2() take struct filename").
> 
> This behaves like do_unlinkat() and do_renameat2().
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
> ---
>  fs/internal.h |  1 +
>  fs/namei.c    | 25 +++++++++++++++++++------
>  2 files changed, 20 insertions(+), 6 deletions(-)

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.

Christian

Re: [PATCH v3 2/2] io_uring: add support for IORING_OP_MKDIRAT

[Al Viro]

On Tue, Mar 30, 2021 at 12:59:57PM +0700, Dmitry Kadashev wrote:
> IORING_OP_MKDIRAT behaves like mkdirat(2) and takes the same flags
> and arguments.

had the questions about interplay with audit been resolved?

Re: [PATCH v3 2/2] io_uring: add support for IORING_OP_MKDIRAT

[Dmitry Kadashev]

On Wed, Mar 31, 2021 at 8:32 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> had the questions about interplay with audit been resolved?

Not to my knowledge. It's just Jens asked to rebase on for-5.13. I'll defer to
Jens to discuss / decide whether adding new ops before the audit side is fixed
worth it or not.

-- 
Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
<christian.brauner@ubuntu.com> 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.

OK, I can change the rest of them in the next version if no one objects.

-- 
Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
<christian.brauner@ubuntu.com> 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?

Thanks.

-- 
Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

On Thu, Apr 8, 2021 at 3:45 PM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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.

The best I could come up with is something like getname_linkat(), which
could be used by the do_linkat callers, but this sounds error prone and
ugly.

Jens, do you want to keep the mkdir change out of 5.13 because of this?

-- 
Dmitry Kadashev

On Thu, Apr 8, 2021 at 3:45 PM Dmitry Kadashev <dkadashev@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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?
>
> Thanks.
>
> --
> Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner]

On Thu, Apr 15, 2021 at 02:14:17PM +0700, Dmitry Kadashev wrote:
> On Thu, Apr 8, 2021 at 3:45 PM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> >
> > On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> > <christian.brauner@ubuntu.com> 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?

From 7adeec2fe4a954e4e4b8a158a4d9fe705b82b978 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner@ubuntu.com>
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 <christian.brauner@ubuntu.com>
---
 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

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner]

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 <dkadashev@gmail.com> wrote:
> > >
> > > On Tue, Mar 30, 2021 at 2:17 PM Christian Brauner
> > > <christian.brauner@ubuntu.com> 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 <christian.brauner@ubuntu.com>
> 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 <christian.brauner@ubuntu.com>
> ---
>  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
> 

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

On Thu, Apr 15, 2021 at 5:09 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Apr 15, 2021 at 12:08:20PM +0200, Christian Brauner wrote:
> > Would something like this help?

Thanks for the reply, Christian!

But it's not the AT_EMPTY_PATH / LOOKUP_EMPTY part that is tricky, it's
the fact that do_linkat() allows AT_EMPTY_PATH only if the process has
CAP_DAC_READ_SEARCH capability. But AT_EMPTY_PATH is processed during
getname(), so if do_linkat() accepts struct filename* then there is no
bullet-proof way to force the capability.

We could do something like this:

do_linkat(oldfd, getname_uflags(oldname, flags), newfd,
          getname(newname), flags);

I.e. call getname_uflags() without checking the capability and rely on
the fact that do_linkat() will do the checking. But this is fragile if
somehow someone passes different flags to getname_uflags and do_linkat.
And there is no way (that I know of) for do_linkat to actually check
that AT_EMPTY_PATH was not used if it gets struct filename.

Or am I creating extra problems and the thing above is OK?


--
Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner]

On Thu, Apr 15, 2021 at 05:41:07PM +0700, Dmitry Kadashev wrote:
> On Thu, Apr 15, 2021 at 5:09 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Thu, Apr 15, 2021 at 12:08:20PM +0200, Christian Brauner wrote:
> > > Would something like this help?
> 
> Thanks for the reply, Christian!
> 
> But it's not the AT_EMPTY_PATH / LOOKUP_EMPTY part that is tricky, it's
> the fact that do_linkat() allows AT_EMPTY_PATH only if the process has
> CAP_DAC_READ_SEARCH capability. But AT_EMPTY_PATH is processed during
> getname(), so if do_linkat() accepts struct filename* then there is no
> bullet-proof way to force the capability.
> 
> We could do something like this:
> 
> do_linkat(oldfd, getname_uflags(oldname, flags), newfd,
>           getname(newname), flags);
> 
> I.e. call getname_uflags() without checking the capability and rely on
> the fact that do_linkat() will do the checking. But this is fragile if
> somehow someone passes different flags to getname_uflags and do_linkat.
> And there is no way (that I know of) for do_linkat to actually check
> that AT_EMPTY_PATH was not used if it gets struct filename.
> 
> Or am I creating extra problems and the thing above is OK?

Hm, I get your point but if you e.g. look at fs/exec.c we already do
have that problem today:

 SYSCALL_DEFINE5(execveat,
		int, fd, const char __user *, filename,
		const char __user *const __user *, argv,
		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),
			   argv, envp, flags);
}

The new simple flag helper would simplify things because right now it
pretends that it cares about multiple flags where it actually just cares
about whether or not empty pathnames are allowed and it forces callers
to translate between flags too.

(Note, just my opinion this might get shot to hell.)

Christian

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev]

On Thu, Apr 15, 2021 at 9:09 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> Hm, I get your point but if you e.g. look at fs/exec.c we already do
> have that problem today:
>
>  SYSCALL_DEFINE5(execveat,
>                 int, fd, const char __user *, filename,
>                 const char __user *const __user *, argv,
>                 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),
>                            argv, envp, flags);
> }
>
> The new simple flag helper would simplify things because right now it
> pretends that it cares about multiple flags where it actually just cares
> about whether or not empty pathnames are allowed and it forces callers
> to translate between flags too.

Hi Christian,

Sorry for the long silence, I got overwhelmed by the primary job and life
stuff. I've finally carved out some time to work on this. I left out the
"make getname_flags accept a single boolean instead of flags" bit to
make the change smaller. If you think it's something that definitely
should be in this patch set then let me know, I'll put it back in. I'm
still somewhat concerned about the separation of the capability check
and the actual logic to get the name, but I guess I'll just post what I
have and collect comments.

I'll send the v4 soon.

-- 
Dmitry Kadashev

Re: [PATCH v3 1/2] fs: make do_mkdirat() take struct filename

[Christian Brauner]

On Thu, May 13, 2021 at 02:45:39PM +0700, Dmitry Kadashev wrote:
> On Thu, Apr 15, 2021 at 9:09 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > Hm, I get your point but if you e.g. look at fs/exec.c we already do
> > have that problem today:
> >
> >  SYSCALL_DEFINE5(execveat,
> >                 int, fd, const char __user *, filename,
> >                 const char __user *const __user *, argv,
> >                 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),
> >                            argv, envp, flags);
> > }
> >
> > The new simple flag helper would simplify things because right now it
> > pretends that it cares about multiple flags where it actually just cares
> > about whether or not empty pathnames are allowed and it forces callers
> > to translate between flags too.
> 
> Hi Christian,
> 
> Sorry for the long silence, I got overwhelmed by the primary job and life
> stuff. I've finally carved out some time to work on this. I left out the

No problem at all! Yeah, I can relate. :)

> "make getname_flags accept a single boolean instead of flags" bit to
> make the change smaller. If you think it's something that definitely
> should be in this patch set then let me know, I'll put it back in. I'm
> still somewhat concerned about the separation of the capability check
> and the actual logic to get the name, but I guess I'll just post what I
> have and collect comments.

Sounds good!

Christian