On Fri, Jul 17, 2020 at 8:44 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote: > On 2020-07-05 11:11, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote: > > > > > > Add audit container identifier auxiliary record to user event standalone > > > records. > > > > > > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx> > > > Acked-by: Neil Horman <nhorman@xxxxxxxxxxxxx> > > > Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > > > --- > > > 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
