All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for stable 5.4] io_uring: Fix current->fs handling in io_sq_wq_submit_work()
@ 2021-01-27 13:34 Nicolai Stange
  0 siblings, 0 replies; only message in thread
From: Nicolai Stange @ 2021-01-27 13:34 UTC (permalink / raw)
  To: stable; +Cc: Jens Axboe, Nicolai Stange

No upstream commit, this is a fix to a stable 5.4 specific backport.

The intention of backport commit cac68d12c531 ("io_uring: grab ->fs as part
of async offload") as found in the stable 5.4 tree was to make
io_sq_wq_submit_work() to switch the workqueue task's ->fs over to the
submitting task's one during the IO operation.

However, due to a small logic error, this change turned out to not have any
actual effect. From a high level, the relevant code in
io_sq_wq_submit_work() looks like

  old_fs_struct = current->fs;
  do {
          ...
          if (req->fs != current->fs && current->fs != old_fs_struct) {
                  task_lock(current);
                  if (req->fs)
                          current->fs = req->fs;
                  else
                          current->fs = old_fs_struct;
                  task_unlock(current);
          }
          ...
  } while (req);

The if condition is supposed to cover the case that current->fs doesn't
match what's needed for processing the request, but observe how it fails
to ever evaluate to true due to the second clause:
current->fs != old_fs_struct will be false in the first iteration as per
the initialization of old_fs_struct and because this prevents current->fs
from getting replaced, the same follows inductively for all subsequent
iterations.

Fix said if condition such that
- if req->fs is set and doesn't match current->fs, the latter will be
  switched to the former
- or if req->fs is unset, the switch back to the initial old_fs_struct
  will be made, if necessary.

While at it, also correct the condition for the ->fs related cleanup right
before the return of io_sq_wq_submit_work(): currently, old_fs_struct is
restored only if it's non-NULL. It is always non-NULL though and thus, the
if-condition is rendundant. Supposedly, the motivation had been to optimize
and avoid switching current->fs back to the initial old_fs_struct in case
it is found to have the desired value already. Make it so.

Cc: stable@vger.kernel.org # v5.4
Fixes: cac68d12c531 ("io_uring: grab ->fs as part of async offload")
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
Tested on top of v5.4.90.

 fs/io_uring.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 4127ea027a14..478df7e10767 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2226,7 +2226,8 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		/* Ensure we clear previously set non-block flag */
 		req->rw.ki_flags &= ~IOCB_NOWAIT;
 
-		if (req->fs != current->fs && current->fs != old_fs_struct) {
+		if ((req->fs && req->fs != current->fs) ||
+		    (!req->fs && current->fs != old_fs_struct)) {
 			task_lock(current);
 			if (req->fs)
 				current->fs = req->fs;
@@ -2351,7 +2352,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		mmput(cur_mm);
 	}
 	revert_creds(old_cred);
-	if (old_fs_struct) {
+	if (old_fs_struct != current->fs) {
 		task_lock(current);
 		current->fs = old_fs_struct;
 		task_unlock(current);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] only message in thread

only message in thread, other threads:[~2021-01-27 13:37 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 13:34 [PATCH for stable 5.4] io_uring: Fix current->fs handling in io_sq_wq_submit_work() Nicolai Stange

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.