linux-kernel - Re: [PATCH 2/2] orangefs: fix double-unlock issue in service

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200523054117.GY23230@ZenIV.linux.org.uk>
Date:   Sat, 23 May 2020 06:41:17 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     wu000273@....edu
Cc:     hubcap@...ibond.com, martin@...ibond.com, devel@...ts.orangefs.org,
        linux-kernel@...r.kernel.org, kjlu@....edu
Subject: Re: [PATCH 2/2] orangefs: fix double-unlock issue in
 service_operation().

On Fri, May 22, 2020 at 11:35:51PM -0500, wu000273@....edu wrote:
> From: Qiushi Wu <wu000273@....edu>
> 
> spin_unlock(&op->lock) is called before calling wake_up_interruptible().
> But spin_unlock() was called again after a call of the function
> "wait_for_matching_downcall" failed.

Yes, it was.

> Fix this issue by remove
> the second spin_unlock().

Why is that a bug?  That's not an idle question - you could demonstrate
that if you had reproduced an unbalanced unlock experimentally, or you
could've proven it possible by analysis of the source.

The former ought to be clearly reported; the latter... AFAICS, your
reasoning is
	1) at the time of wait_for_matching_downcall() call the spinlock
is not being held, since we'd unlocked it upstream of that call and had
done nothing that could have reacquired it.
	2) after the return from that function we are doing unlock.
That is a bug, because one should not unlock a spinlock that is not
locked.

The gap in that proof is the unverified assumption that the locking
conditions upon return from wait_for_matching_downcall() are the same
as upon its call.  IF that assumption holds, there is, indeed a bug.
Now, a look at the function in question shows
	* a comment right before it claiming that it
" * Returns with op->lock taken.".  Which might or might not be correct.
	* one of the wait_for_completion...() called; that clearly
indicates that no spinlocks should be held upon the entry.
	* unconditional spin_lock(&op->lock); right after that.
	* several predicates checked, apparently some debugging
output possibly produced and a value returned.  The predicates
(op_state_service(), op_state_purged()) are clearly locking-neutral -
grep shows
fs/orangefs/orangefs-kernel.h:154:#define op_state_serviced(op)    ((op)->op_state & OP_VFS_STATE_SERVICED)
fs/orangefs/orangefs-kernel.h:155:#define op_state_purged(op)      ((op)->op_state & OP_VFS_STATE_PURGED)
so it's plain arithmetics.  The same, of course, applies to
comparisons.

In other words, the function *does* acquire that spinlock and
does not release it, regardless of the value it returns.  Which
means that your patch would very likely to cause deadlocks.