netdev - Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Aug 2020 14:34:57 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     nhorman@...driver.com, linux-api@...r.kernel.org,
        containers@...ts.linux-foundation.org,
        LKML <linux-kernel@...r.kernel.org>, dhowells@...hat.com,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        netfilter-devel@...r.kernel.org, ebiederm@...ssion.com,
        simo@...hat.com, netdev@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, Eric Paris <eparis@...isplace.org>,
        mpatel@...hat.com, Serge Hallyn <serge@...lyn.com>
Subject: Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records

On Fri, Jul 17, 2020 at 8:44 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2020-07-05 11:11, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <rgb@...hat.com> wrote:
> > >
> > > Add audit container identifier auxiliary record to user event standalone
> > > records.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > > Acked-by: Neil Horman <nhorman@...driver.com>
> > > Reviewed-by: Ondrej Mosnacek <omosnace@...hat.com>
> > > ---
> > >  kernel/audit.c | 19 ++++++++++++-------
> > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 54dd2cb69402..997c34178ee8 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1507,6 +1504,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > >                                 audit_log_n_untrustedstring(ab, str, data_len);
> > >                         }
> > >                         audit_log_end(ab);
> > > +                       rcu_read_lock();
> > > +                       cont = _audit_contobj_get(current);
> > > +                       rcu_read_unlock();
> > > +                       audit_log_container_id(context, cont);
> > > +                       rcu_read_lock();
> > > +                       _audit_contobj_put(cont);
> > > +                       rcu_read_unlock();
> > > +                       audit_free_context(context);
> >
> > I haven't searched the entire patchset, but it seems like the pattern
> > above happens a couple of times in this patchset, yes?  If so would it
> > make sense to wrap the above get/log/put in a helper function?
>
> I've redone the locking with an rcu lock around the get and a spinlock
> around the put.  It occurs to me that putting an rcu lock around the
> whole thing and doing a get without the refcount increment would save
> us the spinlock and put and be fine since we'd be fine with stale but
> consistent information traversing the contobj list from this point to
> report it.  Problem with that is needing to use GFP_ATOMIC due to the
> rcu lock.  If I stick with the spinlock around the put then I can use
> GFP_KERNEL and just grab the spinlock while traversing the contobj list.
>
> > Not a big deal either way, I'm pretty neutral on it at this point in
> > the patchset but thought it might be worth mentioning in case you
> > noticed the same and were on the fence.
>
> There is only one other place this is used, in audit_log_exit in
> auditsc.c.  I had noted the pattern but wasn't sure it was worth it.
> Inline or not?  Should we just let the compiler decide?

I'm generally not a fan of explicit inlines unless it has been shown
to be a real problem.

-- 
paul moore
www.paul-moore.com