Re: [PATCH] netfilter: use per-cpu recursive lock (v11)

[Linus Torvalds <torvalds@linux-foundation.org>]

Ok, so others already pointed out how dangerous/buggy this looks, but I'd 
like to strengthen that a bit more:

On Mon, 20 Apr 2009, Stephen Hemminger wrote:
> +
> +/**
> + * xt_table_info_rdlock_bh - recursive read lock for xt table info
> + *
> + * Table processing calls this to hold off any changes to table
> + * (on current CPU). Always leaves with bottom half disabled.
> + * If called recursively, then assumes bh/preempt already disabled.
> + */
> +void xt_info_rdlock_bh(void)
> +{
> +	struct xt_info_lock *lock;
> +
> +	preempt_disable();
> +	lock = &__get_cpu_var(xt_info_locks);
> +	if (likely(++lock->depth == 0))
> +		spin_lock_bh(&lock->lock);
> +	preempt_enable_no_resched();
> +}
> +EXPORT_SYMBOL_GPL(xt_info_rdlock_bh);

This function is FUCKED UP.

It's total crap for several reasons"

 - the already-mentioned race with bottom half locking.

   If bottom halfs aren't already disabled, then if a bottom half comes in 
   after the "++lock->depth" and before the spin_lock_bh(), then you will 
   have no locking AT ALL for the processing of that bottom half - it will 
   just increment the lock depth again, and nobody will have locked 
   anything at all.

   And if for some reason, you can prove that bottom half processing is 
   already disabled, then ALL THAT OTHER CRAP is just that - CRAP. The 
   whole preemption disabling, the whole "_bh()" thing, everything.

   So either it's horribly buggy, or it's horribly broken and pointless. 
   Take your pick.

 - the total lack of comments. Why does that "count" protect anything? 
   It's not a recursive lock, since there is no ownership (two 
   independent accessors could call this and both "get" the lock), so you 
   had damn well better create some big-ass comments about why it's ok in 
   this case, and what the rules are that make it ok.

   So DON'T GO AROUND CALLING IT A RECURSIVE LOCK! Don't write comments 
   that are TOTAL AND UTTER SH*T! Just DON'T!

   It's a "reader lock". It's not "recursive".  It never was recursive, it 
   never will be, and calling it that is just a sign that whoever wrote 
   the function is a moron and doesn't know what he is doing. STOP DOING THIS!

 - that _idiotic_ "preempt_enable_no_resched()". F*ck me, but the comment 
   already says that preemption is disabled when exiting, so why does it 
   even bother to enable it? Why play those mindless games with preemption 
   counts, knowing that they are bogus?

   Do it readably. Disable preemption first, and just re-enable it at 
   UNLOCK time. No odd pseudo-reenables anywhere.

Oh, I know very well that the spli-locking will disable preemption, so 
it's "correct" to play those games, but the point is, it's just damn 
stupid and annoying. It just makes the source code actively _misleading_ 
to see crap like that - it looks like you enabled preemption, when in fact 
the code very much on purpose does _not_ enable preemption at all. 

In other words, I really REALLY hate that patch. I think it looks slightly 
better than Eric Dumazet's original patch (at least the locking subtlety 
is now in a function of its own and _could_ be commented upon sanely and 
if it wasn't broken it might be acceptable), but quite frankly, I'd still 
horribly disgusted with the crap.

Why are you doing this insane thing? If you want a read-lock, just use the 
damned read-write locks already! Ad far as I can tell, this lock is in 
_no_ way better than just using those counting reader-writer locks, except 
it is open-coded and looks buggy.

There is basically _never_ a good reason to re-implement locking 
primitives: you'll just introduce bugs. As proven very ably by the amount 
of crap above in what is supposed to be a very simple function.

If you want a counting read-lock (in _order_ to allow recursion, but not 
because the lock itself is somehow recursive!), then that function should 
look like this:

	void xt_info_rdlock_bh(void)
	{
		struct xt_info_lock *lock

		local_bh_disable();
		lock = &__get_cpu_var(xt_info_locks);
		read_lock(&lock->lock);
	}

And then the "unlock" should be the reverse. No games, no crap, and 
hopefully then no bugs. And if you do it that way, you don't even need the 
comments, although quite frankly, it probably makes a lot of sense to talk 
about the interaction between "local_bh_disable()" and the preempt count, 
and why you're not using "read_lock_bh()".

And if you don't want a read-lock, then fine - don't use a read-lock, do 
something else. But then don't just re-implement it (badly) either and 
call it something else!

			Linus

PS: Ingo, why do the *_bh() functions in kernel/spinlock.c do _both_ a 
"local_bh_disable()" and a "preempt_disable()"? BH disable should disable 
preemption too, no? Or am I confused? In which case we need that in 
the above rdlock_bh too.