Semantics vs. usage of mutex_is_locked()

Semantics vs. usage of mutex_is_locked()

[Ondrej Mosnacek]

Hello,

(This is addressed mainly to the kernel/locking/ maintainers.)

In security/selinux/ima.c, we have two functions for which we want to
assert the expected locking status of a mutex. In the first function
we expect the caller to obtain the lock, so we have
`WARN_ON(!mutex_is_locked(&state->policy_mutex));` there. The second
one, on the contrary, takes the lock on its own, so there is an
inverse assert (that the caller hasn't already taken the lock) -
`WARN_ON(mutex_is_locked(&state->policy_mutex));`.

Recently, I got a report that the second WARN_ON() got triggered,
while there was no function in the call chain that could have taken
the lock. Looking into it, I realized that mutex_is_locked() actually
doesn't check what we assumed ("Are we holding the lock?"), but
instead answers the question "Is any task holding the lock?". So in
theory it can happen that the second WARN_ON() gets hit randomly in an
otherwise correct code simply because some other task happens to be
holding the mutex. Similarly, the first assert might not catch all
cases where taking the mutex was forgotten, because another task may
be holding it, making the assert pass.

Grepping the whole tree for mutex_is_locked finds about 300 uses, the
vast majority of which are variations of the
warn-if-mutex-not-locked-by-us pattern. Then there are a handful of
cases where the usage of mutex_is_locked() seems correct and a few
cases of the inverse warn-if-mutex-already-locked-by-us pattern.

It seems like introducing a new helper with the "is the mutex locked
by current task?" semantics would be fairly straightforward, however
fixing all the mutex_is_locked() misuses would be a rather big and
noisy patch(set). That said, would it be okay if I send patches that
introduce a new helper and only fix misuses that can lead to wrong
behavior when the code is correct (e.g. can yield a false positive
WARNING/BUG) and documentation? That should be a reasonably small set
of changes, yet should take care of the most important issues. If
anyone cares enough for the rest, they can always send further
patches.

Also, any opinions on the name of the new helper? Perhaps
mutex_is_held()? Or mutex_is_locked_by_current()?

Thanks,

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: Semantics vs. usage of mutex_is_locked()

[Peter Zijlstra]

On Mon, Feb 07, 2022 at 04:15:27PM +0100, Ondrej Mosnacek wrote:
> Also, any opinions on the name of the new helper? Perhaps
> mutex_is_held()? Or mutex_is_locked_by_current()?

lockdep_assert_held*() and friends work on mutexes just fine.

Re: Semantics vs. usage of mutex_is_locked()

[Ondrej Mosnacek]

On Mon, Feb 7, 2022 at 4:47 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Feb 07, 2022 at 04:15:27PM +0100, Ondrej Mosnacek wrote:
> > Also, any opinions on the name of the new helper? Perhaps
> > mutex_is_held()? Or mutex_is_locked_by_current()?
>
> lockdep_assert_held*() and friends work on mutexes just fine.

Hm, good point, although it will only work if CONFIG_LOCKDEP=y, which
is usually not enabled on production kernels... But maybe it doesn't
make sense to do the check on non-debug kernels anyway?

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.