[PATCHSET 0/4] Allow relative lookups

[PATCHSET 0/4] Allow relative lookups

[Jens Axboe]

Due to an oversight on my part, AT_FDCWD lookups only work when the
lookup can be done inline, not async. This patchset rectifies that,
aiming for 5.6 for this one as it would be a shame to have openat etc
without that.

Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
passing it in and setting it, and finally add a ->needs_fs to the opcode
table list of requirements for openat/openat2/statx.

Last patch just ensures we allow AT_FDCWD.

 fs/io-wq.c    | 19 +++++++++++++++----
 fs/io-wq.h    |  4 +++-
 fs/io_uring.c | 29 ++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 6 deletions(-)

-- 
Jens Axboe

[PATCH 1/4] io_uring: grab owning task fs_struct

[Jens Axboe]

We need this for work that operates on relative paths.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ba97bb433922..da6a5998fa30 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -75,6 +75,7 @@
 #include <linux/fsnotify.h>
 #include <linux/fadvise.h>
 #include <linux/eventpoll.h>
+#include <linux/fs_struct.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/io_uring.h>
@@ -264,6 +265,8 @@ struct io_ring_ctx {
 
 	const struct cred	*creds;
 
+	struct fs_struct	*fs;
+
 	/* 0 is for ctx quiesce/reinit/free, 1 is for sqo_thread started */
 	struct completion	*completions;
 
@@ -6261,6 +6264,16 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	if (ctx->account_mem)
 		io_unaccount_mem(ctx->user,
 				ring_pages(ctx->sq_entries, ctx->cq_entries));
+	if (ctx->fs) {
+		struct fs_struct *fs = ctx->fs;
+
+		spin_lock(&ctx->fs->lock);
+		if (--fs->users)
+			fs = NULL;
+		spin_unlock(&ctx->fs->lock);
+		if (fs)
+			free_fs_struct(fs);
+	}
 	free_uid(ctx->user);
 	put_cred(ctx->creds);
 	kfree(ctx->completions);
@@ -6768,6 +6781,13 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p)
 	ctx->user = user;
 	ctx->creds = get_current_cred();
 
+	task_lock(current);
+	spin_lock(&current->fs->lock);
+	ctx->fs = current->fs;
+	ctx->fs->users++;
+	spin_unlock(&current->fs->lock);
+	task_unlock(current);
+
 	ret = io_allocate_scq_urings(ctx, p);
 	if (ret)
 		goto err;
-- 
2.25.0

[PATCH 2/4] io-wq: add support for inheriting ->fs

[Jens Axboe]

Some work items need this for relative path lookup, make it available
like the other inherited credentials/mm/etc.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io-wq.c | 19 +++++++++++++++----
 fs/io-wq.h |  4 +++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/io-wq.c b/fs/io-wq.c
index cb60a42b9fdf..58b1891bcfe5 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/kthread.h>
 #include <linux/rculist_nulls.h>
+#include <linux/fs_struct.h>
 
 #include "io-wq.h"
 
@@ -59,6 +60,7 @@ struct io_worker {
 	const struct cred *cur_creds;
 	const struct cred *saved_creds;
 	struct files_struct *restore_files;
+	struct fs_struct *restore_fs;
 };
 
 #if BITS_PER_LONG == 64
@@ -141,13 +143,17 @@ static bool __io_worker_unuse(struct io_wqe *wqe, struct io_worker *worker)
 		worker->cur_creds = worker->saved_creds = NULL;
 	}
 
-	if (current->files != worker->restore_files) {
+	if ((current->files != worker->restore_files) ||
+	    (current->fs != worker->restore_fs)) {
 		__acquire(&wqe->lock);
 		spin_unlock_irq(&wqe->lock);
 		dropped_lock = true;
 
 		task_lock(current);
-		current->files = worker->restore_files;
+		if (current->files != worker->restore_files)
+			current->files = worker->restore_files;
+		if (current->fs != worker->restore_fs)
+			current->fs = worker->restore_fs;
 		task_unlock(current);
 	}
 
@@ -311,6 +317,7 @@ static void io_worker_start(struct io_wqe *wqe, struct io_worker *worker)
 
 	worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING);
 	worker->restore_files = current->files;
+	worker->restore_fs = current->fs;
 	io_wqe_inc_running(wqe, worker);
 }
 
@@ -476,9 +483,13 @@ static void io_worker_handle_work(struct io_worker *worker)
 		if (work->flags & IO_WQ_WORK_CB)
 			work->func(&work);
 
-		if (work->files && current->files != work->files) {
+		if ((work->files && current->files != work->files) ||
+		    (work->fs && current->fs != work->fs)) {
 			task_lock(current);
-			current->files = work->files;
+			if (work->files && current->files != work->files)
+				current->files = work->files;
+			if (work->fs && current->fs != work->fs)
+				current->fs = work->fs;
 			task_unlock(current);
 		}
 		if (work->mm != worker->mm)
diff --git a/fs/io-wq.h b/fs/io-wq.h
index 50b3378febf2..f152ba677d8f 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -74,6 +74,7 @@ struct io_wq_work {
 	struct files_struct *files;
 	struct mm_struct *mm;
 	const struct cred *creds;
+	struct fs_struct *fs;
 	unsigned flags;
 };
 
@@ -81,10 +82,11 @@ struct io_wq_work {
 	do {						\
 		(work)->list.next = NULL;		\
 		(work)->func = _func;			\
-		(work)->flags = 0;			\
 		(work)->files = NULL;			\
 		(work)->mm = NULL;			\
 		(work)->creds = NULL;			\
+		(work)->fs = NULL;			\
+		(work)->flags = 0;			\
 	} while (0)					\
 
 typedef void (get_work_fn)(struct io_wq_work *);
-- 
2.25.0

[PATCH 3/4] io_uring: add ->needs_fs to the opcode requirements table

[Jens Axboe]

Set it for openat/openat2/statx, as they all potentially need relative
path lookups if AT_FDCWD is used.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index da6a5998fa30..957de0f99bcd 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -616,6 +616,8 @@ struct io_op_def {
 	unsigned		not_supported : 1;
 	/* needs file table */
 	unsigned		file_table : 1;
+	/* needs ->fs assigned */
+	unsigned		needs_fs : 1;
 };
 
 static const struct io_op_def io_op_defs[] = {
@@ -694,6 +696,7 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_file		= 1,
 		.fd_non_neg		= 1,
 		.file_table		= 1,
+		.needs_fs		= 1,
 	},
 	[IORING_OP_CLOSE] = {
 		.needs_file		= 1,
@@ -707,6 +710,7 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_mm		= 1,
 		.needs_file		= 1,
 		.fd_non_neg		= 1,
+		.needs_fs		= 1,
 	},
 	[IORING_OP_READ] = {
 		.needs_mm		= 1,
@@ -738,6 +742,7 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_file		= 1,
 		.fd_non_neg		= 1,
 		.file_table		= 1,
+		.needs_fs		= 1,
 	},
 	[IORING_OP_EPOLL_CTL] = {
 		.unbound_nonreg_file	= 1,
@@ -911,6 +916,8 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
 	}
 	if (!req->work.creds)
 		req->work.creds = get_current_cred();
+	if (!req->work.fs && def->needs_fs)
+		req->work.fs = req->ctx->fs;
 }
 
 static inline void io_req_work_drop_env(struct io_kiocb *req)
-- 
2.25.0

[PATCH 4/4] io_uring: allow AT_FDCWD for non-file openat/openat2/statx

[Jens Axboe]

Don't just check for dirfd == -1, we should allow AT_FDCWD as well for
relative lookups.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 957de0f99bcd..b35703582086 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4467,7 +4467,7 @@ static int io_req_needs_file(struct io_kiocb *req, int fd)
 {
 	if (!io_op_defs[req->opcode].needs_file)
 		return 0;
-	if (fd == -1 && io_op_defs[req->opcode].fd_non_neg)
+	if ((fd == -1 || fd == AT_FDCWD) && io_op_defs[req->opcode].fd_non_neg)
 		return 0;
 	return 1;
 }
-- 
2.25.0

Re: [PATCHSET 0/4] Allow relative lookups

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 659 bytes --]

Hi Jens,

Am 07.02.20 um 16:50 schrieb Jens Axboe:
> Due to an oversight on my part, AT_FDCWD lookups only work when the
> lookup can be done inline, not async. This patchset rectifies that,
> aiming for 5.6 for this one as it would be a shame to have openat etc
> without that.
> 
> Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
> passing it in and setting it, and finally add a ->needs_fs to the opcode
> table list of requirements for openat/openat2/statx.
> 
> Last patch just ensures we allow AT_FDCWD.

Thanks! But IOSQE_FIXED_FILE is still not supported and not rejected at
the same time, correct?

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHSET 0/4] Allow relative lookups

[Jens Axboe]

On 2/7/20 2:56 PM, Stefan Metzmacher wrote:
> Hi Jens,
> 
> Am 07.02.20 um 16:50 schrieb Jens Axboe:
>> Due to an oversight on my part, AT_FDCWD lookups only work when the
>> lookup can be done inline, not async. This patchset rectifies that,
>> aiming for 5.6 for this one as it would be a shame to have openat etc
>> without that.
>>
>> Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
>> passing it in and setting it, and finally add a ->needs_fs to the opcode
>> table list of requirements for openat/openat2/statx.
>>
>> Last patch just ensures we allow AT_FDCWD.
> 
> Thanks! But IOSQE_FIXED_FILE is still not supported and not rejected at
> the same time, correct?

That's in a separate patch:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=5e159663813f0b7837342426cfb68185b6609359

-- 
Jens Axboe

Re: [PATCHSET 0/4] Allow relative lookups

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 1320 bytes --]

Hi Jens,

>> Am 07.02.20 um 16:50 schrieb Jens Axboe:
>>> Due to an oversight on my part, AT_FDCWD lookups only work when the
>>> lookup can be done inline, not async. This patchset rectifies that,
>>> aiming for 5.6 for this one as it would be a shame to have openat etc
>>> without that.
>>>
>>> Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
>>> passing it in and setting it, and finally add a ->needs_fs to the opcode
>>> table list of requirements for openat/openat2/statx.
>>>
>>> Last patch just ensures we allow AT_FDCWD.
>>
>> Thanks! But IOSQE_FIXED_FILE is still not supported and not rejected at
>> the same time, correct?
> 
> That's in a separate patch:
> 
> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=5e159663813f0b7837342426cfb68185b6609359

Do we handle the error path correct?
As far as I can see io_req_set_file() is called before
io_{statx,openat,openat2}_prep() and req->file is already filled.
Maybe a generic way would be better using io_op_defs[op].allow_fixed_file.

In the long run we may want to add support for openat2 with
IOSQE_FIXED_FILE, then Samba could register the share root directory as
fixed file and it could be used for all openat2 calls.
But for now it's fine to just reject it...

Thanks!
metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHSET 0/4] Allow relative lookups

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 1367 bytes --]

Am 07.02.20 um 23:56 schrieb Stefan Metzmacher:
> Hi Jens,
> 
>>> Am 07.02.20 um 16:50 schrieb Jens Axboe:
>>>> Due to an oversight on my part, AT_FDCWD lookups only work when the
>>>> lookup can be done inline, not async. This patchset rectifies that,
>>>> aiming for 5.6 for this one as it would be a shame to have openat etc
>>>> without that.
>>>>
>>>> Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
>>>> passing it in and setting it, and finally add a ->needs_fs to the opcode
>>>> table list of requirements for openat/openat2/statx.
>>>>
>>>> Last patch just ensures we allow AT_FDCWD.
>>>
>>> Thanks! But IOSQE_FIXED_FILE is still not supported and not rejected at
>>> the same time, correct?
>>
>> That's in a separate patch:
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=5e159663813f0b7837342426cfb68185b6609359
> 
> Do we handle the error path correct?
> As far as I can see io_req_set_file() is called before
> io_{statx,openat,openat2}_prep() and req->file is already filled.
> Maybe a generic way would be better using io_op_defs[op].allow_fixed_file.

BTW: I'm really wondering what req->needs_fixed_file is for...

Does it mean that IORING_SETUP_SQPOLL only works with IOSQE_FIXED_FILE?
Which would mean IORING_SETUP_SQPOLL can't support openat2 and others.

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHSET 0/4] Allow relative lookups

[Jens Axboe]

On 2/7/20 3:56 PM, Stefan Metzmacher wrote:
> Hi Jens,
> 
>>> Am 07.02.20 um 16:50 schrieb Jens Axboe:
>>>> Due to an oversight on my part, AT_FDCWD lookups only work when the
>>>> lookup can be done inline, not async. This patchset rectifies that,
>>>> aiming for 5.6 for this one as it would be a shame to have openat etc
>>>> without that.
>>>>
>>>> Just 3 small simple patches - grab the task ->fs, add io-wq suppor for
>>>> passing it in and setting it, and finally add a ->needs_fs to the opcode
>>>> table list of requirements for openat/openat2/statx.
>>>>
>>>> Last patch just ensures we allow AT_FDCWD.
>>>
>>> Thanks! But IOSQE_FIXED_FILE is still not supported and not rejected at
>>> the same time, correct?
>>
>> That's in a separate patch:
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=5e159663813f0b7837342426cfb68185b6609359
> 
> Do we handle the error path correct?
> As far as I can see io_req_set_file() is called before
> io_{statx,openat,openat2}_prep() and req->file is already filled.
> Maybe a generic way would be better using io_op_defs[op].allow_fixed_file.

Worst case, as far as I can tell, is that we'll think it's a valid
descriptor (because the both the fixed index and fd are valid) and we'll
still error in the prep. Only concern would be that maybe we should make
this an -EBADF return, which would be 100% consistent between them (and
with other cases). I'll make that change.

> In the long run we may want to add support for openat2 with
> IOSQE_FIXED_FILE, then Samba could register the share root directory as
> fixed file and it could be used for all openat2 calls.
> But for now it's fine to just reject it...

It's not impossible to support, it's just that it requires changes
outside of io_uring to do so. So for now it'll just not be possible, I'm
afraid.

-- 
Jens Axboe