Re: Deferred freeing of kauth_cred_ts

[Elad Efrat <elad%NetBSD.org@localhost>]

Iain Hibbert wrote:
> On Fri, 25 Dec 2009, Elad Efrat wrote:
>
> > > +       SLIST_INSERT_HEAD(&kauth_thread_discard, cred, cr_list);
> > >
> > > should access to this SLIST be protected by a mutex?  There could be a
> > > race between adding and removing..
> > Yeah... would an IPL_HIGH mutex be fine there? (if I read mutex(9)
> > right, we need a spin mutex for our purposes.)
>
> Mm, I think you are right - guess you only need to hold it while accessing
> the list (drop it before the free), unless there is a better method?

Attached is a diff that adds a spin mutex when accessing the list. (We
release it before the free and reacquire it afterwards.) I'm now testing
it... I also didn't change any of the names etc. yet.

Thanks,

-e.

Index: kern/kern_auth.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_auth.c,v
retrieving revision 1.64
diff -u -p -r1.64 kern_auth.c
--- kern/kern_auth.c    3 Sep 2009 04:45:27 -0000       1.64
+++ kern/kern_auth.c    25 Dec 2009 09:39:41 -0000
@@ -69,6 +69,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_auth.c,
 #include <sys/atomic.h>
 #include <sys/specificdata.h>
 #include <sys/vnode.h>
+#include <sys/kthread.h>
 
 /*
  * Secmodel-specific credentials.
@@ -106,6 +107,7 @@ struct kauth_cred {
        u_int cr_ngroups;               /* number of groups */
        gid_t cr_groups[NGROUPS];       /* group memberships */
        specificdata_reference cr_sd;   /* specific data */
+       SLIST_ENTRY(kauth_cred) cr_list;/* deferred free */
 };
 
 /*
@@ -149,6 +151,11 @@ static unsigned int nsecmodels = 0;
 
 static specificdata_domain_t kauth_domain;
 static pool_cache_t kauth_cred_cache;
+
+static kmutex_t kauth_gc_lock;
+static SLIST_HEAD(,kauth_cred) kauth_thread_discard;
+static lwp_t *kauth_thread_lwp;
+
 krwlock_t      kauth_lock;
 
 /* Allocate new, empty kauth credentials. */
@@ -184,6 +191,15 @@ kauth_cred_hold(kauth_cred_t cred)
         atomic_inc_uint(&cred->cr_refcnt);
 }
 
+static void
+kauth_cred_free_now(kauth_cred_t cred)
+{
+
+       kauth_cred_hook(cred, KAUTH_CRED_FREE, NULL, NULL);
+       specificdata_fini(kauth_domain, &cred->cr_sd);
+       pool_cache_put(kauth_cred_cache, cred);
+}
+
 /* Decrease reference count to cred. If reached zero, free it. */
 void
 kauth_cred_free(kauth_cred_t cred)
@@ -196,9 +212,9 @@ kauth_cred_free(kauth_cred_t cred)
        if (atomic_dec_uint_nv(&cred->cr_refcnt) > 0)
                return;
 
-       kauth_cred_hook(cred, KAUTH_CRED_FREE, NULL, NULL);
-       specificdata_fini(kauth_domain, &cred->cr_sd);
-       pool_cache_put(kauth_cred_cache, cred);
+       mutex_enter(&kauth_gc_lock);
+       SLIST_INSERT_HEAD(&kauth_thread_discard, cred, cr_list);
+       mutex_exit(&kauth_gc_lock);
 }
 
 static void
@@ -786,6 +802,29 @@ kauth_register_scope(const char *id, kau
        return (scope);
 }
 
+static void
+kauth_thread(void *cookie)
+{
+       kauth_cred_t cred;
+       extern int hz;
+
+       KASSERT(curlwp == kauth_thread_lwp);
+
+       for (;;) {
+               mutex_enter(&kauth_gc_lock);
+               while (!SLIST_EMPTY(&kauth_thread_discard)) {
+                       cred = SLIST_FIRST(&kauth_thread_discard);
+                       SLIST_REMOVE_HEAD(&kauth_thread_discard, cr_list);
+                       mutex_exit(&kauth_gc_lock);
+                       kauth_cred_free_now(cred);
+                       mutex_enter(&kauth_gc_lock);
+               }
+               mutex_exit(&kauth_gc_lock);
+
+               kpause("kauthgc", false, mstohz(1000), NULL);
+       }
+}
+
 /*
  * Initialize the kernel authorization subsystem.
  *
@@ -837,6 +876,20 @@ kauth_init(void)
        /* Register vnode scope. */
        kauth_builtin_scope_vnode = kauth_register_scope(KAUTH_SCOPE_VNODE,
            NULL, NULL);
+
+       mutex_init(&kauth_gc_lock, MUTEX_DEFAULT, IPL_HIGH);
+}
+
+
+void
+kauth_start_gc_thread(void)
+{
+       int error;
+
+       error = kthread_create(PRI_NONE, KTHREAD_MPSAFE, NULL, kauth_thread,
+           NULL, &kauth_thread_lwp, "kauthgc");
+       if (error)
+               panic("kauth_start_gc_thread: can't create gc thread: error 
%d", error);
 }
 
 /*
Index: kern/init_main.c
===================================================================
RCS file: /cvsroot/src/sys/kern/init_main.c,v
retrieving revision 1.413
diff -u -p -r1.413 init_main.c
--- kern/init_main.c    23 Dec 2009 00:21:38 -0000      1.413
+++ kern/init_main.c    25 Dec 2009 09:39:41 -0000
@@ -678,6 +678,9 @@ main(void)
            uvm_aiodone_worker, NULL, PRI_VM, IPL_NONE, WQ_MPSAFE))
                panic("fork aiodoned");
 
+       /* Start the kauth(9) garbage collector. */
+       kauth_start_gc_thread();
+
        vmem_rehash_start();
 
        /* Initialize exec structures */
Index: sys/kauth.h
===================================================================
RCS file: /cvsroot/src/sys/sys/kauth.h,v
retrieving revision 1.64
diff -u -p -r1.64 kauth.h
--- sys/kauth.h 24 Dec 2009 19:02:07 -0000      1.64
+++ sys/kauth.h 25 Dec 2009 09:39:41 -0000
@@ -341,6 +341,7 @@ enum {
  * Prototypes.
  */
 void kauth_init(void);
+void kauth_start_gc_thread(void);
 kauth_scope_t kauth_register_scope(const char *, kauth_scope_callback_t, void 
*);
 void kauth_deregister_scope(kauth_scope_t);
 kauth_listener_t kauth_listen_scope(const char *, kauth_scope_callback_t, void 
*);