From: "J. Bruce Fields" <bfields@fieldses.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
Al Viro <viro@zeniv.linux.org.uk>,
"Serge E. Hallyn" <serge@hallyn.com>,
Linux-Fsdevel <linux-fsdevel@vger.kernel.org>,
Kernel Mailing List <linux-kernel@vger.kernel.org>,
Andy Lutomirski <luto@amacapital.net>,
Rob Landley <rob@landley.net>,
Linus Torvalds <torvalds@linux-foundation.org>,
Christoph Hellwig <hch@infradead.org>,
Karel Zak <kzak@redhat.com>
Subject: Re: [PATCH 08/11] vfs: Merge check_submounts_and_drop and d_invalidate
Date: Wed, 26 Feb 2014 14:37:40 -0500 [thread overview]
Message-ID: <20140226193740.GA24456@fieldses.org> (raw)
In-Reply-To: <87mwhe26kn.fsf@xmission.com>
On Tue, Feb 25, 2014 at 02:03:36PM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
> > On Mon, Feb 24, 2014 at 04:01:29PM -0800, Eric W. Biederman wrote:
> >> Miklos Szeredi <miklos@szeredi.hu> writes:
> >>
> >> >
> >> > You can optimize this by including the negative check within the above d_locked
> >> > region and calling __d_drop() instead.
> >>
> >> For this patch just moving the code and not changing it is the corret
> >> thing to do because it helps with review and understanding the code.
> >>
> >> There are two ways I could see going with optimizing the preamble.
> >> Simply dropping the d_lock from around the d_unhashed test as a pointer
> >> dereference should be atomic, and the test is racy against
> >> d_materialise_unique.
> >
> > Could you explain? What's the race, and what are the consequences?
Actually I was just confused as to whether the above was "is racy" was
claiming the existance of some bug.
I believe I should have read the above as more like "the test is already
racy against d_materialise_unique, but it's a harmless race, and
dropping the d_lock wouldn't make it any worse".
> >> (We don't always hold the parent directories inode mutex when d_invalidate is called).
>
> d_unhashed is not a permanent condition because of d_materialise_unique,
> and d_splice_alias.
>
> d_invalidate can be called on an unhashed dentry in one of two ways
> (either d_revalidate dropped the dentry or another routine that drops
> the dentry beat the current invocation of d_invalidate to the job).
>
>
> There are 3 places d_revalidate is called.
>
> Once on the rcu path with with the appropriate flag set.
>
> Once without out the parent i_mutex held, just off of the rcu path,
> on that path d_invalidate is when d_revalidate fails.
>
> Once during lookup with the parent directory i_mutex held.
>
>
> Because the parent direcories i_mutex is not always held accross
> d_revalidate and the following d_invalidate it happens that d_invalidate
> is not always an atomic operation.
>
>
> At worst the race results in a dentry that is dropped when it could be
> hashed,
Because somebody not holding the i_mutex calls d_invalidate based on old
information and unhashes something that
d_materialise_unique/d_splice_alias just hashed?
> that we will resurrect next time someone attempts to look it
> up and d_materialise_unique/d_splice_alias is called.
OK.
> None of that really matters for optimizing d_invalidate, but it is part
> of the background in which d_invalidate lives. All that is significant
> in d_invalidate is knowing that d_materialise_unique, and possibly
> d_splice_alias may run concurrently with d_invalidate. It is unlikely
> and essentially harmless.
>
>
> After my patchset (because I removed all of the d_drop's from
> .d_revalidate) the only race that should remain is between two parallel
> calls of d_invalidate. Which probably means we can remove the test for
> d_unhashed altogether.
>
> Right now I just want to make this first big step and make certain the
> code is solid. After that optimization is easy.
Thanks for the explanation!
--b.
next prev parent reply other threads:[~2014-02-26 19:38 UTC|newest]
Thread overview: 165+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-14 17:42 DoS with unprivileged mounts Miklos Szeredi
2013-08-14 19:26 ` Andy Lutomirski
2013-08-14 19:53 ` Eric W. Biederman
2013-08-14 20:25 ` Andy Lutomirski
2013-08-14 21:54 ` Eric W. Biederman
2013-08-14 19:32 ` Eric W. Biederman
2013-08-15 4:59 ` Miklos Szeredi
2013-08-15 6:45 ` Eric W. Biederman
2013-08-15 6:52 ` Andy Lutomirski
2013-08-15 7:55 ` Eric W. Biederman
2013-08-15 9:29 ` Miklos Szeredi
2013-10-04 22:41 ` [RFC][PATCH 0/3] vfs: Detach mounts on unlink Eric W. Biederman
2013-10-04 22:42 ` [RFC][PATCH 1/3] vfs: Keep a list of mounts on a mount point Eric W. Biederman
2013-10-08 13:44 ` Miklos Szeredi
2013-10-08 20:31 ` Eric W. Biederman
2013-10-04 22:43 ` [RFC][PATCH 2/3] vfs: Add a function to lazily unmount all mounts from any dentry Eric W. Biederman
2013-10-07 4:22 ` Serge E. Hallyn
2013-10-07 6:15 ` Eric W. Biederman
2013-10-08 13:56 ` Miklos Szeredi
2013-10-08 20:32 ` Eric W. Biederman
2013-10-04 22:43 ` [RFC][PATCH 3/3] vfs: Lazily remove mounts on unlinked files and directories Eric W. Biederman
2013-10-08 15:50 ` Miklos Szeredi
2013-10-08 21:47 ` Eric W. Biederman
2013-10-09 19:12 ` Grrrr fusermount Eric W. Biederman
2013-10-09 20:09 ` Andy Lutomirski
2013-10-09 23:35 ` Eric W. Biederman
2013-10-04 23:20 ` [RFC][PATCH 0/3] vfs: Detach mounts on unlink Linus Torvalds
2013-10-05 0:03 ` Eric W. Biederman
2013-10-05 1:57 ` Eric W. Biederman
2013-10-05 23:42 ` Rob Landley
2013-10-06 0:44 ` Eric W. Biederman
2013-10-08 8:03 ` Karel Zak
2013-10-10 6:46 ` Rob Landley
2013-10-05 2:34 ` [RFC][PATCH 4/3] vfs: Allow rmdir to remove mounts in all but the current mount namespace Eric W. Biederman
2013-10-05 15:44 ` Serge E. Hallyn
2013-10-07 4:39 ` Serge E. Hallyn
2013-10-07 6:55 ` Eric W. Biederman
2013-10-07 16:53 ` Andy Lutomirski
2013-10-07 22:25 ` Eric W. Biederman
2013-10-08 16:06 ` Miklos Szeredi
2013-10-08 16:06 ` Andy Lutomirski
2013-10-08 16:11 ` Miklos Szeredi
2013-10-08 20:50 ` Eric W. Biederman
2013-10-10 10:02 ` Miklos Szeredi
2013-10-10 11:43 ` Eric W. Biederman
2013-10-10 11:57 ` Miklos Szeredi
2013-10-12 1:04 ` Eric W. Biederman
2013-10-12 1:39 ` Eric W. Biederman
2013-10-15 20:15 ` [REVIEW][PATCH 0/4] vfs: Detach mounts on unlink Eric W. Biederman
2013-10-15 20:16 ` [REVIEW][PATCH 1/4] vfs: Don't allow overwriting mounts in the current mount namespace Eric W. Biederman
2013-10-22 19:04 ` Serge E. Hallyn
2013-11-03 3:54 ` Al Viro
2013-11-08 20:51 ` Eric W. Biederman
2013-11-08 21:35 ` Al Viro
2013-11-08 22:17 ` Eric W. Biederman
2013-11-09 8:49 ` Christoph Hellwig
2013-11-21 20:58 ` Eric W. Biederman
2013-11-21 20:49 ` Eric W. Biederman
2013-10-15 20:17 ` [REVIEW][PATCH 2/4] vfs: Keep a list of mounts on a mount point Eric W. Biederman
2013-10-22 19:06 ` Serge E. Hallyn
2013-10-15 20:17 ` [REVIEW][PATCH 3/4] vfs: Add a function to lazily unmount all mounts from any dentry. v3 Eric W. Biederman
2013-10-22 19:08 ` Serge E. Hallyn
2013-10-15 20:18 ` [REVIEW][PATCH 4/4] vfs: Lazily remove mounts on unlinked files and directories. v2 Eric W. Biederman
2013-10-22 19:13 ` Serge E. Hallyn
2013-10-08 10:42 ` [RFC][PATCH 4/3] vfs: Allow rmdir to remove mounts in all but the current mount namespace Matthias Schniedermeyer
2013-10-08 13:13 ` Eric W. Biederman
2013-10-05 23:07 ` [RFC][PATCH 0/3] vfs: Detach mounts on unlink Rob Landley
2013-10-05 23:17 ` Linus Torvalds
2013-10-05 23:22 ` Linus Torvalds
2013-10-06 0:18 ` Rob Landley
2013-10-06 0:37 ` Linus Torvalds
2013-10-05 23:24 ` Al Viro
2013-10-06 0:22 ` Rob Landley
2013-10-06 0:12 ` Rob Landley
2013-10-05 23:19 ` Al Viro
2013-10-06 0:13 ` Rob Landley
2014-02-15 21:34 ` [PATCH 0/11] Detaching " Eric W. Biederman
2014-02-15 21:35 ` [PATCH 01/11] vfs: Document the effect of d_revalidate on d_find_alias Eric W. Biederman
2014-02-15 21:36 ` [PATCH 02/11] vfs: More precise tests in d_invalidate Eric W. Biederman
2014-02-15 22:51 ` Linus Torvalds
2014-02-15 22:59 ` Linus Torvalds
2014-02-15 23:23 ` Eric W. Biederman
2014-02-15 23:39 ` Eric W. Biederman
2014-02-16 0:03 ` Linus Torvalds
2014-02-16 1:22 ` Eric W. Biederman
2014-02-15 21:36 ` [PATCH 03/11] vfs: Don't allow overwriting mounts in the current mount namespace Eric W. Biederman
2014-02-18 17:12 ` Miklos Szeredi
2014-02-18 21:28 ` Eric W. Biederman
2014-02-18 22:20 ` Linus Torvalds
2014-02-19 2:23 ` Stephen Rothwell
2014-02-24 23:43 ` Eric W. Biederman
2014-02-15 21:37 ` [PATCH 04/11] vfs: Keep a list of mounts on a mount point Eric W. Biederman
2014-02-15 21:37 ` [PATCH 05/11] vfs: Add a function to lazily unmount all mounts from any dentry Eric W. Biederman
2014-02-16 1:50 ` Eric W. Biederman
2014-02-18 17:17 ` Miklos Szeredi
2014-02-25 3:24 ` Eric W. Biederman
2014-02-15 21:38 ` [PATCH 06/11] vfs: Lazily remove mounts on unlinked files and directories Eric W. Biederman
2014-02-15 21:38 ` [PATCH 07/11] vfs: Remove unnecessary calls of check_submounts_and_drop Eric W. Biederman
2014-02-15 21:39 ` [PATCH 08/11] vfs: Merge check_submounts_and_drop and d_invalidate Eric W. Biederman
2014-02-18 17:40 ` Miklos Szeredi
2014-02-25 0:01 ` Eric W. Biederman
2014-02-25 15:13 ` J. Bruce Fields
2014-02-25 22:03 ` Eric W. Biederman
2014-02-26 19:37 ` J. Bruce Fields [this message]
2014-02-27 2:05 ` Eric W. Biederman
2014-02-15 21:39 ` [PATCH 09/11] vfs: Make d_invalidate return void Eric W. Biederman
2014-02-15 21:40 ` [PATCH 10/11] vfs: Remove d_drop calls from d_revalidate implementations Eric W. Biederman
2014-02-15 21:40 ` [PATCH 11/11] proc: Update proc_flush_task_mnt to use d_invalidate Eric W. Biederman
2014-02-18 17:43 ` [PATCH 0/11] Detaching mounts on unlink Miklos Szeredi
2014-02-25 9:33 ` [PATCH 0/12] Detaching mounts on unlink (for 3.15 v2) Eric W. Biederman
2014-02-25 9:34 ` [PATCH 01/12] vfs: Document the effect of d_revalidate on d_find_alias Eric W. Biederman
2014-02-25 9:35 ` [PATCH 02/12] vfs: More precise tests in d_invalidate Eric W. Biederman
2014-02-25 9:35 ` [PATCH 03/12] vfs: Don't allow overwriting mounts in the current mount namespace Eric W. Biederman
2014-02-25 9:36 ` [PATCH 04/12] vfs: Keep a list of mounts on a mount point Eric W. Biederman
2014-02-25 9:37 ` [PATCH 05/12] vfs: factor out lookup_mountpoint from new_mountpoint Eric W. Biederman
2014-02-25 9:38 ` [PATCH 06/12] vfs: Add a function to lazily unmount all mounts from any dentry Eric W. Biederman
2014-02-25 9:38 ` [PATCH 07/12] vfs: Lazily remove mounts on unlinked files and directories Eric W. Biederman
2014-02-25 9:39 ` [PATCH 08/12] vfs: Remove unnecessary calls of check_submounts_and_drop Eric W. Biederman
2014-02-25 9:40 ` [PATCH 09/12] vfs: Merge check_submounts_and_drop and d_invalidate Eric W. Biederman
2014-02-25 9:40 ` [PATCH 10/12] vfs: Make d_invalidate return void Eric W. Biederman
2014-02-25 9:41 ` [PATCH 11/12] vfs: Remove d_drop calls from d_revalidate implementations Eric W. Biederman
2014-02-25 9:41 ` [PATCH 12/12] proc: Update proc_flush_task_mnt to use d_invalidate Eric W. Biederman
2014-04-09 0:21 ` [GIT PULL] Detaching mounts on unlink for 3.15-rc1 Eric W. Biederman
2014-04-09 2:30 ` Al Viro
2014-04-09 2:39 ` Al Viro
2014-04-09 9:02 ` Eric W. Biederman
2014-04-09 17:32 ` Eric W. Biederman
2014-04-09 17:53 ` Al Viro
2014-04-09 18:28 ` Al Viro
2014-04-09 22:49 ` Eric W. Biederman
2014-04-09 22:58 ` [RFC][PATCH] vfs: In mntput run deactivate_super on a shallow stack Eric W. Biederman
2014-04-09 23:24 ` Al Viro
2014-04-10 1:36 ` Eric W. Biederman
2014-04-12 22:15 ` Eric W. Biederman
2014-04-13 5:39 ` Al Viro
2014-04-13 5:49 ` Al Viro
2014-04-13 7:01 ` Eric W. Biederman
2014-04-13 7:51 ` Eric W. Biederman
2014-04-13 21:52 ` Al Viro
2014-04-14 7:38 ` [RFC][PATCH 0/4] No I/O from mntput Eric W. Biederman
2014-04-14 7:40 ` [PATCH 1/4] vfs: Remove useless loop in mntput_no_expire Eric W. Biederman
2014-04-14 7:40 ` [PATCH 2/4] vfs: Move autoclose of BSD accounting into a work queue Eric W. Biederman
2014-04-14 7:41 ` [PATCH 3/4] vfs: In mntput run deactivate_super on a shallow stack Eric W. Biederman
2014-04-14 7:42 ` [PATCH 4/4] vfs: Block intuitively in the case of BSD accounting files Eric W. Biederman
2014-04-16 22:03 ` [RFC][PATCH 0/4] No I/O from mntput Eric W. Biederman
2014-04-17 20:05 ` [GIT PULL] Detaching mounts on unlink for 3.15 Eric W. Biederman
2014-04-17 20:22 ` Al Viro
2014-04-17 21:23 ` Eric W. Biederman
2014-04-17 22:12 ` Al Viro
2014-04-17 22:14 ` Al Viro
2014-04-18 0:37 ` Al Viro
2014-04-18 0:58 ` Al Viro
2014-04-19 1:35 ` Al Viro
2014-04-19 2:16 ` Al Viro
2014-04-19 7:05 ` Al Viro
2014-04-20 5:41 ` Al Viro
2014-05-11 16:45 ` Al Viro
2014-05-11 17:05 ` Al Viro
2014-08-09 9:34 ` Al Viro
2014-08-09 9:55 ` Eric W. Biederman
2014-08-12 10:17 ` Eric W. Biederman
2014-08-13 13:18 ` Al Viro
2014-08-13 17:18 ` Eric W. Biederman
2014-04-09 22:01 ` [GIT PULL] Detaching mounts on unlink for 3.15-rc1 Dave Chinner
2013-08-15 21:23 ` DoS with unprivileged mounts Rob Landley
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=20140226193740.GA24456@fieldses.org \
--to=bfields@fieldses.org \
--cc=ebiederm@xmission.com \
--cc=hch@infradead.org \
--cc=kzak@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=miklos@szeredi.hu \
--cc=rob@landley.net \
--cc=serge@hallyn.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).