On Tue, Feb 20, 2018 at 09:51:08AM -0500, Paul Moore wrote:
On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra peterz@infradead.org wrote:
It's not at all clear to me what that code does, I just stumbled upon __mutex_owner() outside of the mutex code itself and went WTF.
If you don't want people to use __mutex_owner() outside of the mutex code I might suggest adding a rather serious comment at the top of the function, because right now I don't see anything suggesting that function shouldn't be used. Yes, there is the double underscore prefix, but that can mean a few different things these days.
Find below.
The comment (aside from having the most horribly style) ...
Yeah, your dog is ugly too. Notice how neither comment is constructive?
I'm sure you've seen this one:
https://lkml.org/lkml/2016/7/8/625
It's all about reading code; inconsistent and unbalanced styles are just _really_ hard on the brain.
... is wrong too, because it claims it will not block when we hold that lock, while, afaict, it will in fact do just that.
A mutex blocks when it is held, but the audit_log_start() function should not block for the task that currently holds the audit_cmd_mutex; that is what the comment is meant to convey. I believe the comment makes sense, but I did write it so I'll concede that I'm probably the not best judge. If anyone would like to offer a different wording I'm happy to consider it.
The comment uses 'sleep' which is typically used to mean anything that schedules, but then it does the schedule_timeout() thing.
Maybe if you could explain how that code is supposed to work and why it doesn't know if it holds a lock I could make a suggestion...
I just spent a few minutes looking back over the bits available in include/linux/mutex.h and I'm not seeing anything beyond __mutex_owner() which would allow us to determine the mutex owning task. It's probably easiest for us to just track ownership ourselves. I'll put together a patch later today.
Note that up until recently the mutex implementation didn't even have a consistent owner field. And the thing is, it's very easy to use wrong, only today I've seen a patch do: "__mutex_owner() == task", where task was allowed to be !current, which is just wrong.
Looking through kernel/audit.c I'm not even sure I see how you would end up in audit_log_start() with audit_cmd_mutex held.
Can you give me a few code paths that trigger this? Simple git-grep is failing me.
--- Subject: mutex: Add comment to __mutex_owner() From: Peter Zijlstra peterz@infradead.org Date: Tue Feb 20 16:01:36 CET 2018
Attempt to deter usage, this is not a public interface. It is entirely possibly to implement a conformant mutex without having this owner field (in fact, we used to have that).
Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org --- --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -66,6 +66,11 @@ struct mutex { #endif };
+/* + * Internal helper function; C doesn't allow us to hide it :/ + * + * DO NOT USE (outside of mutex code). + */ static inline struct task_struct *__mutex_owner(struct mutex *lock) { return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);