All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: linux-kernel@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org, criu@openvz.org,
	bpf@vger.kernel.org,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Christian Brauner" <christian.brauner@ubuntu.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"Cyrill Gorcunov" <gorcunov@gmail.com>,
	"Jann Horn" <jann@thejh.net>, "Kees Cook" <keescook@chromium.org>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Jeff Layton" <jlayton@redhat.com>,
	"Miklos Szeredi" <miklos@szeredi.hu>,
	"Matthew Wilcox" <willy@debian.org>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	"Matthew Wilcox" <matthew@wil.cx>,
	"Trond Myklebust" <trond.myklebust@fys.uio.no>,
	"Chris Wright" <chrisw@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andriin@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@chromium.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>
Subject: [PATCH 12/17] proc/fd: In fdinfo seq_show don't use get_files_struct
Date: Mon, 17 Aug 2020 17:04:20 -0500	[thread overview]
Message-ID: <20200817220425.9389-12-ebiederm@xmission.com> (raw)
In-Reply-To: <87ft8l6ic3.fsf@x220.int.ebiederm.org>

When discussing[1] exec and posix file locks it was realized that none
of the callers of get_files_struct fundamentally needed to call
get_files_struct, and that by switching them to helper functions
instead it will both simplify their code and remove unnecessary
increments of files_struct.count.  Those unnecessary increments can
result in exec unnecessarily unsharing files_struct which breaking
posix locks, and it can result in fget_light having to fallback to
fget reducing system performance.

Instead hold task_lock for the duration that task->files needs to be
stable in seq_show.  The task_lock was already taken in
get_files_struct, and so skipping get_files_struct performs less work
overall, and avoids the problems with the files_struct reference
count.

[1] https://lkml.kernel.org/r/20180915160423.GA31461@redhat.com
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/proc/fd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index d9fee5390fd7..0b46eea154b7 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -28,9 +28,8 @@ static int seq_show(struct seq_file *m, void *v)
 	if (!task)
 		return -ENOENT;
 
-	files = get_files_struct(task);
-	put_task_struct(task);
-
+	task_lock(task);
+	files = task->files;
 	if (files) {
 		unsigned int fd = proc_fd(m->private);
 
@@ -47,8 +46,9 @@ static int seq_show(struct seq_file *m, void *v)
 			ret = 0;
 		}
 		spin_unlock(&files->file_lock);
-		put_files_struct(files);
 	}
+	task_unlock(task);
+	put_task_struct(task);
 
 	if (ret)
 		return ret;
@@ -57,6 +57,7 @@ static int seq_show(struct seq_file *m, void *v)
 		   (long long)file->f_pos, f_flags,
 		   real_mount(file->f_path.mnt)->mnt_id);
 
+	/* show_fd_locks() never deferences files so a stale value is safe */
 	show_fd_locks(m, file, files);
 	if (seq_has_overflowed(m))
 		goto out;
-- 
2.25.0


  parent reply	other threads:[~2020-08-17 22:12 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-17 22:03 exec: Move unshare_files and guarantee files_struct.count is correct Eric W. Biederman
2020-08-17 22:04 ` [PATCH 01/17] exec: Move unshare_files to fix posix file locking during exec Eric W. Biederman
2020-08-18 10:04   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 02/17] exec: Simplify unshare_files Eric W. Biederman
2020-08-18 10:08   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 03/17] exec: Remove reset_files_struct Eric W. Biederman
2020-08-18 10:09   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 04/17] kcmp: In kcmp_epoll_target use fget_task Eric W. Biederman
2020-08-20 21:45   ` Cyrill Gorcunov
2020-08-17 22:04 ` [PATCH 05/17] bpf: In bpf_task_fd_query " Eric W. Biederman
2020-08-17 22:04 ` [PATCH 06/17] file: Implement fcheck_task Eric W. Biederman
2020-08-18 10:37   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 07/17] proc/fd: In tid_fd_mode use fcheck_task Eric W. Biederman
2020-08-18 10:36   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 08/17] proc/fd: In proc_fd_link " Eric W. Biederman
2020-08-18 10:36   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 09/17] file: Implement fnext_task Eric W. Biederman
2020-08-17 23:54   ` Linus Torvalds
     [not found]     ` <875z9g7oln.fsf@x220.int.ebiederm.org>
2020-08-18  1:17       ` Linus Torvalds
2020-08-18 11:05         ` Christian Brauner
     [not found]           ` <87pn7m22kn.fsf@x220.int.ebiederm.org>
2020-08-19 15:54             ` Alexei Starovoitov
     [not found]               ` <871rk0t45v.fsf@x220.int.ebiederm.org>
2020-08-21 16:17                 ` Alexei Starovoitov
2020-08-19 18:32             ` Linus Torvalds
2020-08-20 21:50   ` Cyrill Gorcunov
2020-08-17 22:04 ` [PATCH 10/17] proc/fd: In proc_readfd_common use fnext_task Eric W. Biederman
2020-08-18  2:22   ` Al Viro
     [not found]     ` <87sgck4o23.fsf@x220.int.ebiederm.org>
2020-08-18  4:59       ` Alexei Starovoitov
2020-08-17 22:04 ` [PATCH 11/17] bpf/task_iter: In task_file_seq_get_next " Eric W. Biederman
2020-08-18  5:39   ` kernel test robot
2020-08-18  5:39     ` kernel test robot
2020-08-18 12:54     ` Eric W. Biederman
2020-08-18 12:54       ` Eric W. Biederman
2020-08-17 22:04 ` Eric W. Biederman [this message]
2020-08-18  0:08   ` [PATCH 12/17] proc/fd: In fdinfo seq_show don't use get_files_struct Linus Torvalds
     [not found]     ` <87v9hg69ph.fsf@x220.int.ebiederm.org>
2020-08-18  1:21       ` Linus Torvalds
2020-08-18 10:43   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 13/17] file: Remove get_files_struct Eric W. Biederman
2020-08-18 10:39   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 14/17] file: Merge __fd_install into fd_install Eric W. Biederman
2020-08-18 10:15   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 15/17] file: In f_dupfd read RLIMIT_NOFILE once Eric W. Biederman
2020-08-18 10:12   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 16/17] file: Merge __alloc_fd into alloc_fd Eric W. Biederman
2020-08-18 10:17   ` Christian Brauner
2020-08-17 22:04 ` [PATCH 17/17] file: Rename __close_fd to close_fd and remove the files parameter Eric W. Biederman
2020-08-18 10:19   ` Christian Brauner
2020-08-18 11:20   ` Christoph Hellwig
2020-08-18 12:48     ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200817220425.9389-12-ebiederm@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=berrange@redhat.com \
    --cc=bfields@fieldses.org \
    --cc=bpf@vger.kernel.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=chrisw@redhat.com \
    --cc=criu@openvz.org \
    --cc=daniel@iogearbox.net \
    --cc=gorcunov@gmail.com \
    --cc=jann@thejh.net \
    --cc=jlayton@redhat.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=keescook@chromium.org \
    --cc=kpsingh@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew@wil.cx \
    --cc=miklos@szeredi.hu \
    --cc=oleg@redhat.com \
    --cc=songliubraving@fb.com \
    --cc=torvalds@linux-foundation.org \
    --cc=trond.myklebust@fys.uio.no \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@debian.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.