Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra <peterz(a)infradead.org> wrote:
On Tue, Feb 20, 2018 at 08:25:21AM -0500, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra <peterz(a)infradead.org> wrote:
> > On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
> >> 4.10-stable review patch. If anyone has any objections, please let me
know.
> >
> >> + if (!(auditd_test_task(current) ||
> >> + (current == __mutex_owner(&audit_cmd_mutex)))) {
> >> + long stime = audit_backlog_wait_time;
> >
> > Since I cannot find the original email on lkml, NAK on this.
> > __mutex_owner() is not a general purpose helper function.
>
> Since this code also exists in the current kernel, I need to ask what
> recommended alternatives exist for determining the mutex owner?
>
> I imagine we could track the mutex owner separately in the audit
> subsystem, but I'd much prefer to leverage an existing mechanism if
> possible.
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.
The comment (aside from having the most horribly style) ...
Yeah, your dog is ugly too. Notice how neither comment is constructive?
... 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.
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.
--
paul moore
www.paul-moore.com