From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Christian Brauner <christian.brauner@ubuntu.com>,
Oleg Nesterov <oleg@redhat.com>, Jann Horn <jann@thejh.net>
Subject: Re: [PATCH] files: rcu free files_struct
Date: Wed, 9 Dec 2020 19:50:33 +0000 [thread overview]
Message-ID: <20201209195033.GP3579531@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CAHk-=wiUMHBHmmDS3_Xqh1wfGFyd_rdDmpZzk0cODoj1i7_VOA@mail.gmail.com>
On Wed, Dec 09, 2020 at 11:13:38AM -0800, Linus Torvalds wrote:
> On Wed, Dec 9, 2020 at 10:05 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >
> > - struct file * file = xchg(&fdt->fd[i], NULL);
> > + struct file * file = fdt->fd[i];
> > if (file) {
> > + rcu_assign_pointer(fdt->fd[i], NULL);
>
> This makes me nervous. Why did we use to do that xchg() there? That
> has atomicity guarantees that now are gone.
>
> Now, this whole thing should be called for just the last ref of the fd
> table, so presumably that atomicity was never needed in the first
> place. But the fact that we did that very expensive xchg() then makes
> me go "there's some reason for it".
>
> Is this xchg() just bogus historical leftover? It kind of looks that
> way. But maybe that change should be done separately?
I'm still not convinced that exposing close_files() to parallel
3rd-party accesses is safe in all cases, so this patch still needs
more analysis. And I'm none too happy about "we'll fix the things
up at the tail of the series" - the changes are subtle enough and
the area affected is rather fundamental. So if we end up returning
to that several years from now while debugging something, I would
very much prefer to have the transformation series as clean and
understandable as possible. It's not just about bisect hazard -
asking yourself "WTF had it been done that way, is there anything
subtle I'm missing here?" can cost many hours of head-scratching,
IME.
Eric, I understand that you want to avoid reordering/folding, but
in this case it _is_ needed. It's not as if there had been any
serious objections to the overall direction of changes; it's
just that we need to get that as understandable as possible.
next prev parent reply other threads:[~2020-12-09 19:51 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <87r1on1v62.fsf@x220.int.ebiederm.org>
2020-11-20 23:14 ` [PATCH v2 01/24] exec: Move unshare_files to fix posix file locking during exec Eric W. Biederman
2020-11-20 23:58 ` Linus Torvalds
2020-12-07 22:22 ` Al Viro
[not found] ` <87zh2pusdw.fsf@x220.int.ebiederm.org>
2020-12-07 23:35 ` Al Viro
2020-11-20 23:14 ` [PATCH v2 02/24] exec: Simplify unshare_files Eric W. Biederman
2020-11-23 17:50 ` Oleg Nesterov
2020-11-23 18:25 ` Linus Torvalds
[not found] ` <87im9vx08i.fsf@x220.int.ebiederm.org>
[not found] ` <87pn42r0n7.fsf@x220.int.ebiederm.org>
2020-11-24 19:58 ` Linus Torvalds
2020-11-24 20:14 ` Arnd Bergmann
2020-11-24 23:44 ` Geoff Levand
2020-11-25 1:16 ` Eric W. Biederman
2020-11-27 20:29 ` Arnd Bergmann
2020-11-30 21:37 ` Eric W. Biederman
2020-12-01 9:46 ` Michael Ellerman
2020-11-25 21:51 ` [RFC][PATCH] coredump: Document coredump code exclusively used by cell spufs Eric W. Biederman
2020-12-02 15:20 ` Eric W. Biederman
2020-12-02 15:58 ` Arnd Bergmann
2020-12-07 22:32 ` [PATCH v2 02/24] exec: Simplify unshare_files Al Viro
2020-11-20 23:14 ` [PATCH v2 03/24] exec: Remove reset_files_struct Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 04/24] kcmp: In kcmp_epoll_target use fget_task Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 05/24] bpf: In bpf_task_fd_query " Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 06/24] proc/fd: In proc_fd_link " Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 07/24] file: Rename __fcheck_files to files_lookup_fd_raw Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 08/24] file: Factor files_lookup_fd_locked out of fcheck_files Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 09/24] file: Replace fcheck_files with files_lookup_fd_rcu Eric W. Biederman
2020-12-07 22:46 ` Al Viro
2020-12-07 22:49 ` Al Viro
2020-12-09 16:54 ` Al Viro
2020-12-09 17:44 ` Paul E. McKenney
2020-11-20 23:14 ` [PATCH v2 10/24] file: Rename fcheck lookup_fd_rcu Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 11/24] file: Implement task_lookup_fd_rcu Eric W. Biederman
2020-11-21 18:19 ` Cyrill Gorcunov
[not found] ` <87blfp1r8b.fsf@x220.int.ebiederm.org>
2020-11-22 13:52 ` Cyrill Gorcunov
2020-11-20 23:14 ` [PATCH v2 12/24] proc/fd: In tid_fd_mode use task_lookup_fd_rcu Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 13/24] kcmp: In get_file_raw_ptr " Eric W. Biederman
2020-11-23 21:05 ` Cyrill Gorcunov
2020-11-20 23:14 ` [PATCH v2 14/24] file: Implement task_lookup_next_fd_rcu Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 15/24] proc/fd: In proc_readfd_common use task_lookup_next_fd_rcu Eric W. Biederman
2020-12-07 23:29 ` Al Viro
[not found] ` <877dprvs8e.fsf@x220.int.ebiederm.org>
2020-12-09 4:07 ` Al Viro
[not found] ` <877dprtxly.fsf@x220.int.ebiederm.org>
2020-12-09 14:23 ` Al Viro
2020-12-09 18:04 ` [PATCH] files: rcu free files_struct Eric W. Biederman
2020-12-09 19:13 ` Linus Torvalds
2020-12-09 19:50 ` Al Viro [this message]
2020-12-09 21:32 ` Eric W. Biederman
2020-12-10 6:13 ` Al Viro
2020-12-10 19:29 ` Eric W. Biederman
2020-12-10 21:36 ` Al Viro
2020-12-10 22:30 ` Christian Brauner
2020-12-10 22:54 ` Al Viro
2020-12-10 23:10 ` Al Viro
2020-12-09 21:42 ` [PATCH -1/24] exec: Don't open code get_close_on_exec Eric W. Biederman
2020-12-09 22:04 ` [PATCH] files: rcu free files_struct Eric W. Biederman
2020-12-09 19:49 ` Matthew Wilcox
2020-12-09 22:06 ` Eric W. Biederman
2020-12-09 22:58 ` Al Viro
2020-12-09 23:01 ` Linus Torvalds
2020-12-09 23:04 ` Linus Torvalds
2020-12-09 23:07 ` Matthew Wilcox
2020-12-09 23:26 ` Paul E. McKenney
2020-12-09 23:29 ` Linus Torvalds
2020-12-10 0:39 ` Paul E. McKenney
2020-12-10 0:41 ` Linus Torvalds
2020-12-10 0:57 ` Paul E. McKenney
2020-11-20 23:14 ` [PATCH v2 16/24] bpf/task_iter: In task_file_seq_get_next use task_lookup_next_fd_rcu Eric W. Biederman
2020-11-23 17:06 ` Yonghong Song
2020-11-20 23:14 ` [PATCH v2 17/24] proc/fd: In fdinfo seq_show don't use get_files_struct Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 18/24] file: Merge __fd_install into fd_install Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 19/24] file: In f_dupfd read RLIMIT_NOFILE once Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 20/24] file: Merge __alloc_fd into alloc_fd Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 21/24] file: Rename __close_fd to close_fd and remove the files parameter Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 22/24] file: Replace ksys_close with close_fd Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 23/24] file: Rename __close_fd_get_file close_fd_get_file Eric W. Biederman
2020-11-20 23:14 ` [PATCH v2 24/24] file: Remove get_files_struct Eric W. Biederman
2020-11-21 0:05 ` [PATCH v2 00/24] exec: Move unshare_files and guarantee files_struct.count is correct Linus Torvalds
2020-11-28 5:12 ` Al Viro
2020-12-07 23:56 ` Al Viro
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=20201209195033.GP3579531@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=christian.brauner@ubuntu.com \
--cc=ebiederm@xmission.com \
--cc=jann@thejh.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=torvalds@linux-foundation.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).