Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

On Tue, Feb 20, 2018 at 10:18 AM, Peter Zijlstra <peterz(a)infradead.org&gt; wrote: > 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(a)infradead.org&gt; 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 Yep. I stand behind my earlier comment in this thread. >> > 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. Arguably all the more reason why a strongly worded warning is important (which I see you've included below, feel free to include my Reviewed-by). > 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. Basically look at the code in audit_receive_msg(), but I wasn't asking your opinion on how we should rewrite the audit subsystem, I was just asking how one could determine if the current task was holding a given mutex in a way that was acceptable to you. Based on your comments, and some further inspection of the mutex code, it appears that is/was not something that the core mutex code wants to support/make-visible. Which is perfectly fine, I just wanted to make sure I wasn't missing something before I went ahead and wrote a wrapper around the mutex code for use by audit. FWIW, I just put together the following patch which removes the __mutex_owner() call from audit and doesn't appear to break anything on the audit side (you're CC'd on the patch). It has only been lightly tested, but I'm going to bang on it for a day or so and if I hear no objections I'll merge it into audit/next. * https://www.redhat.com/archives/linux-audit/2018-February/msg00066.html