Re: [PATCH RESEND] fs:Fix rcu locking in the function terminate_walk

Re: [PATCH RESEND] fs:Fix rcu locking in the function terminate_walk

[Al Viro]

On Tue, Dec 29, 2015 at 08:30:19PM -0500, Nicholas Krause wrote:
> This fixes rcu locking for the else block in the function
> terminate_walk by calling rcu_read_lock at the beginning
> of this code in order to protect against concurrent writers
> here in this else block while allowing multiple readers to
> be able to read concurrently here.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>

Patch is complete garbage, and so's the vague "commit message".
"concurrent writers"?  Writers to _what_?  We do not touch *ANY*
potentially shared data objects from that point to the place where
rcu_read_unlock() is done.  All the 3 lines until rcu_read_unlock().

And yes, I can reconstruct your "thinking process" here - playing
psychoproctologist isn't hard, just unpleasant:
	* Og see rcu_read_unlock()
	* Og see no rcu_read_lock()
	* Og add rcu_read_lock()
	* Og slap boilerplate "explanation" thingy in "commit message" thingy
	* Og send
	* Og contributed

What you have not done after the second step is

	* post "why is that rcu_read_unlock() in terminate_walk()
unpaired?"

*OR* if you are willing to spend a few minutes to answer that question
yourself:

	* check if rcu_read_lock() might have been already done
	* look around for apparently unpaired rcu_read_lock and rcu_read_unlock
	* see a couple of such rcu_read_lock() in different cases of
path_init() and several rcu_read_unlock() in different cases of unlazy_walk(),
unlazy_link() and terminate_walk()
	* note that these rcu_read_lock() in path_init() happen when
there's LOOKUP_RCU in flags
	* see that it happens in *all* success cases of path_init() with
LOOKUP_RCU in flags and only in those.
	* guess that LOOKUP_RCU might have something to do with that,
especially since that case in terminate_walk() is *also* after finding
LOOKUP_RCU, only in nd->flags this time.  And it's followed by removing
LOOKUP_RCU from there.
	* observe that nd->flags is set by path_init() and LOOKUP_RCU
ends up there exactly in cases when rcu_read_lock() had been done.
	* look where else do we remove LOOKUP_RCU.  See it done in
unlazy_walk(), unlazy_link() - exact same places where we have apparently
unpaired rcu_read_unlock().
	* observe that rcu_read_unlock() happens on all paths in
unlazy_walk(), and LOOKUP_RCU removal is unconditional there.
	* observe that rcu_read_unlock() and LOOKUP_RCU removal in
unlazy_link() happen in the same branch, right next to each other (the
other branch calls unlazy_walk())
	* come to obvious hypothesis - that LOOKUP_RCU in nd->flags corresponds
to doing the pathname resolution under rcu_read_lock() and dropping
rcu_read_lock() goes along with removing LOOKUP_RCU from there.
	* verify it (observations above cover most of that work, but rechecking
with explicit hypothesis to verify doesn't hurt).

You don't change your MO, do you?  You see something, don't spend any effort
trying to understand what's there or asking what's going on; instead you
post random crap patches, let the subsystem maintainers deal with it,
nod thanks to any explanations of the reasons you were wrong and go on
doing the same fucking thing again and again, despite any attempts of
explanations.  When people start filtering you out, you come up with yet
another email address and continue the same damn thing from it.

In your world it apparently constitutes "contributions" and you don't
give a flying fuck about any evidence to the contrary...

*plonk*
*plonk*