On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs [off-list ref] wrote:

On 2020-07-05 11:09, Paul Moore wrote:

quoted

On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs [off-list ref] wrote:

...

quoted

quoted

@@ -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...

quoted

quoted

+               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()?

It reads okay to me, but I know what I'm intending here :)  I agree it
could be a bit confusing.  Let me try to put my suggestion into some
quick pseudo-code function prototypes to make things a bit more
concrete.

The _audit_contobj_hold() function would become:
   struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk);

The _audit_contobj_get() function would become:
   struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont);

The _audit_contobj_put() function would become:
   void _audit_contobj_put(struct audit_contobj *cont);

Basically swap the _get() and _hold() function names so that the
arguments are the same for both the _get() and _set() functions.  Does
this make more sense?

quoted

quoted

 /**
  * 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.

Sorry, you can disregard.  My mental definition for audit_alloc() is
permanently messed up; I usually double check myself before commenting
on related code, but I must have forgotten here.

-- 
paul moore
www.paul-moore.com