lkml.org 
[lkml]   [2018]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
    On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra <peterz@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@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

    \
     
     \ /
      Last update: 2018-02-20 15:51    [W:4.659 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site