From: ebiederm@xmission.com (Eric W. Biederman)
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
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, 09 Dec 2020 15:32:38 -0600 [thread overview]
Message-ID: <87sg8er7gp.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20201209195033.GP3579531@ZenIV.linux.org.uk> (Al Viro's message of "Wed, 9 Dec 2020 19:50:33 +0000")
Al Viro <viro@zeniv.linux.org.uk> writes:
> 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.
That is fine. I just wanted to post the latest version so we could
continue the discussion. Especially with comments etc.
> 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.
Fair enough. I don't expect anyone is basing anything on that branch,
so a rebase is possible.
Now removing the pounding on task_lock isn't about correctness, and it
is not fixing a performance problem anyone has measured at this point.
So I do think it should be a follow on. If for no other reason than to
keep the problem small enough it can fit in heads.
Similarly the dnotify stuff. Your description certain makes it look
fishy but that the questionable parts are orthogonal to my patches.
> 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.
I will post the patch that will become -1/24 in a moment.
Eric
next prev parent reply other threads:[~2020-12-09 21:34 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
2020-12-09 21:32 ` Eric W. Biederman [this message]
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=87sg8er7gp.fsf@x220.int.ebiederm.org \
--to=ebiederm@xmission.com \
--cc=christian.brauner@ubuntu.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 \
--cc=viro@zeniv.linux.org.uk \
/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).