Re: [PATCH ghak90 V9 02/13] audit: add container id

On 2020-07-05 11:09, Paul Moore wrote: > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote:

> > @@ -212,6 +219,33 @@ void __init audit_task_init(void) > > 0, SLAB_PANIC, NULL); > > } > > > > +/* rcu_read_lock must be held by caller unless new */ > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj *cont) > > +{ > > + if (cont) > > + refcount_inc(&cont->refcount); > > + return cont; > > +} > > + > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk) > > +{ > > + if (!tsk->audit) > > + return NULL; > > + return _audit_contobj_hold(tsk->audit->cont); > > +} > > + > > +/* rcu_read_lock must be held by caller */ > > +static void _audit_contobj_put(struct audit_contobj *cont) > > +{ > > + if (!cont) > > + return; > > + if (refcount_dec_and_test(&cont->refcount)) { > > + put_task_struct(cont->owner); > > + list_del_rcu(&cont->list); > > You should check your locking; I'm used to seeing exclusive locks > (e.g. the spinlock) around list adds/removes, it just reads/traversals > that can be done with just the RCU lock held. Ok, I've redone the locking yet again. I knew this on one level but that didn't translate consistently to code... > > + kfree_rcu(cont, rcu); > > + } > > +} > > Another nitpick, but it might be nice to have similar arguments to the > _get() and _put() functions, e.g. struct audit_contobj, but that is > some serious bikeshedding (basically rename _hold() to _get() and > rename _hold to audit_task_contid_hold() or similar). I have some idea what you are trying to say, but I think you misspoke. Did you mean rename _hold to _get, rename _get to audit_task_contobj_hold()?

> > /** > > * audit_alloc - allocate an audit info block for a task > > * @tsk: task > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk) > > } > > info->loginuid = audit_get_loginuid(current); > > info->sessionid = audit_get_sessionid(current); > > + rcu_read_lock(); > > + info->cont = _audit_contobj_get(current); > > + rcu_read_unlock(); > > The RCU locks aren't strictly necessary here, are they? In fact I > suppose we could probably just replace the _get() call with a > refcount_set(1) just as we do in audit_set_contid(), yes? I don't understand what you are getting at here. It needs a *contobj, along with bumping up the refcount of the existing contobj.