Re: [PATCH 02/46] fs: d_validate fixes
- From: Nick Piggin <npiggin@xxxxxxxxx>
- Date: Thu, 9 Dec 2010 15:50:17 +1100
On Thu, Dec 09, 2010 at 11:50:29AM +1100, Dave Chinner wrote:
On Wed, Dec 08, 2010 at 05:59:55PM +1100, Nick Piggin wrote:
On Wed, Dec 08, 2010 at 12:53:44PM +1100, Dave Chinner wrote:
On Sat, Nov 27, 2010 at 08:44:32PM +1100, Nick Piggin wrote:
d_validate has been broken for a long time.
kmem_ptr_validate does not guarantee that a pointer can be dereferenced
if it can go away at any time. Even rcu_read_lock doesn't help, because
the pointer might be queued in RCU callbacks but not executed yet.
So the parent cannot be checked, nor the name hashed. The dentry pointer
can not be touched until it can be verified under lock. Hashing simply
cannot be used.
Instead, verify the parent/child relationship by traversing parent's
d_child list. It's slow, but only ncpfs and the destaged smbfs care
about it, at this point.
I'd drop the previous revert patch and just convert the RCU hash
traversal straight to the d_child traversal code you introduce here.
This is a much better explanation of why the d_validate mechanism
needs to be changed, and the revert is really an unnecessary extra
step...
Has to be backported, though.
Backported where? The d_validate() change only got included in .37-rc1.
Backported to stable/distro kernels I suppose. I'm not sure what your
point is?
Patch that is to be reverted obviously
adds more brokenness and is a good example that you cannot dget() under
rcu read protection even if the rest of the surrounding function is
bugfree. I wouldn't have thought it's a big deal.
Reverting something broken to something already broken just to fix
to the less broken version seems like an unnecessary step. Just
fix the brokenneѕs in a single patch - no need to indirect the real
fix through a revert. One less patch to worry about.
OK but I disagree. Firstly, reverting that patch gives a good record of
that particular pattern of bug (that Christoph and Al both missed).
With more RCU going into the vfs, people need to be pretty clear about
the pitfalls.
Secondly, as I said, reverting means that I can use exact same patch
for upstream and stable kernels.
And finally, it gives better bisectability. If somebody hits a bug in
my patch, I would rather have them bisect into the well-worn (if buggy)
version of the code than bisect into a different type of brokenness.
It isn't indirecting the real fix through a revert, they are broken in
different ways. My fix is for the bug that it doesn't guarantee the
persistence of *memory* we are using, and the revert is for the bug that
it doesn't guarantee the persistence/validity of the *object*, and which
is actually more likely to be a problem if you think about it, because
the window is much larger.
Git has no problem with lots of patches, so I don't see any advantage
to doing one patch, and you lose the advantages above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- References:
- Re: [PATCH 02/46] fs: d_validate fixes
- From: Dave Chinner
- Re: [PATCH 02/46] fs: d_validate fixes
- From: Nick Piggin
- Re: [PATCH 02/46] fs: d_validate fixes
- From: Dave Chinner
- Re: [PATCH 02/46] fs: d_validate fixes
- Prev by Date: linux-next: Tree for December 9
- Next by Date: Re: [PATCH] ACPI: fix section mismatch warning
- Previous by thread: Re: [PATCH 02/46] fs: d_validate fixes
- Next by thread: [PATCH 0/2] perf tools: add reference timestamp and use it in time history dump
- Index(es):
Relevant Pages
|
