Subject: FILEASSOC_NHOOKS removal and some questions
To: None <elad@netbsd.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 12/08/2006 23:03:25
--NextPart-20061208225838-1213100
Content-Type: Text/Plain; charset=us-ascii

the attached patch does:

- remove FILEASSOC_NHOOKS.

- make fileassoc_t a pointer and remove FILEASSOC_INVAL.

- clean up kern_fileassoc.c.  unify duplicated code.

- unexport fileassoc_init using RUN_ONCE(9).

- plug memory leaks in fileassoc_file_delete and fileassoc_table_delete.

now, i have questions:

- fileassoc_tabledata_clear doesn't call FILEASSOC_CLEANUP_TABLE callback
  while fileassoc_clear calls FILEASSOC_CLEANUP_FILE.  is it intended?

- when the associated data is NULL, whether cleanup callback is
  invoked or not seems inconsistent among functions.  is it intended?

YAMAMOTO Takashi

--NextPart-20061208225838-1213100
Content-Type: Text/Plain; charset=us-ascii
Content-Disposition: attachment; filename="a.diff"

Index: sys/fileassoc.h
===================================================================
--- sys/fileassoc.h	(revision 1931)
+++ sys/fileassoc.h	(working copy)
@@ -36,17 +36,14 @@
 #include <sys/cdefs.h>
 #include <sys/param.h>
 
-#define	FILEASSOC_INVAL		-1
-
-typedef int fileassoc_t;
+typedef struct fileassoc *fileassoc_t;
 typedef void (*fileassoc_cleanup_cb_t)(void *, int);
 typedef void (*fileassoc_cb_t)(void *);
 
 #define	FILEASSOC_CLEANUP_TABLE	0
 #define	FILEASSOC_CLEANUP_FILE	1
 
-void fileassoc_init(void);
-fileassoc_t fileassoc_register(const char *, fileassoc_cleanup_cb_t);
+int fileassoc_register(const char *, fileassoc_cleanup_cb_t, fileassoc_t *);
 int fileassoc_deregister(fileassoc_t);
 void *fileassoc_tabledata_lookup(struct mount *, fileassoc_t);
 void *fileassoc_lookup(struct vnode *, fileassoc_t);
Index: kern/kern_fileassoc.c
===================================================================
--- kern/kern_fileassoc.c	(revision 1931)
+++ kern/kern_fileassoc.c	(working copy)
@@ -46,32 +46,36 @@ __KERNEL_RCSID(0, "$NetBSD: kern_fileass
 #include <sys/inttypes.h>
 #include <sys/errno.h>
 #include <sys/fileassoc.h>
+#include <sys/specificdata.h>
 #include <sys/hash.h>
 #include <sys/fstypes.h>
-
-/* Max. number of hooks. */
-#ifndef FILEASSOC_NHOOKS
-#define	FILEASSOC_NHOOKS	4
-#endif /* !FILEASSOC_NHOOKS */
+#include <sys/kmem.h>
+#include <sys/once.h>
 
 static struct fileassoc_hash_entry *
 fileassoc_file_lookup(struct vnode *, fhandle_t *);
 static struct fileassoc_hash_entry *
 fileassoc_file_add(struct vnode *, fhandle_t *);
 
+static specificdata_domain_t fileassoc_domain;
+
 /*
  * Hook entry.
  * Includes the hook name for identification and private hook clear callback.
  */
-struct fileassoc_hook {
-	const char *hook_name;			/* Hook name. */
-	fileassoc_cleanup_cb_t hook_cleanup_cb;	/* Hook clear callback. */
+struct fileassoc {
+	LIST_ENTRY(fileassoc) list;
+	const char *name;			/* name. */
+	fileassoc_cleanup_cb_t cleanup_cb;	/* clear callback. */
+	specificdata_key_t key;
 };
 
+static LIST_HEAD(, fileassoc) fileassoc_list;
+
 /* An entry in the per-device hash table. */
 struct fileassoc_hash_entry {
 	fhandle_t *handle;				/* File handle */
-	void *hooks[FILEASSOC_NHOOKS];			/* Hooks. */
+	specificdata_reference data;			/* Hooks. */
 	LIST_ENTRY(fileassoc_hash_entry) entries;	/* List pointer. */
 };
 
@@ -82,13 +86,10 @@ struct fileassoc_table {
 	size_t hash_size;				/* Number of slots. */
 	struct mount *tbl_mntpt;
 	u_long hash_mask;
-	void *tables[FILEASSOC_NHOOKS];
+	specificdata_reference data;
 	LIST_ENTRY(fileassoc_table) hash_list;		/* List pointer. */
 };
 
-struct fileassoc_hook fileassoc_hooks[FILEASSOC_NHOOKS];
-int fileassoc_nhooks;
-
 /* Global list of hash tables, one per device. */
 LIST_HEAD(, fileassoc_table) fileassoc_tables;
 
@@ -100,54 +101,133 @@ LIST_HEAD(, fileassoc_table) fileassoc_t
 	(hash32_buf((handle), FHANDLE_SIZE(handle), HASH32_BUF_INIT) \
 	 & ((tbl)->hash_mask))
 
+static void *
+table_getdata(struct fileassoc_table *tbl, const struct fileassoc *assoc)
+{
+
+	return specificdata_getspecific(fileassoc_domain, &tbl->data,
+	    assoc->key);
+}
+
+static void
+table_setdata(struct fileassoc_table *tbl, const struct fileassoc *assoc,
+    void *data)
+{
+
+	specificdata_setspecific(fileassoc_domain, &tbl->data, assoc->key,
+	    data);
+}
+
+static void
+table_cleanup(struct fileassoc_table *tbl, const struct fileassoc *assoc)
+{
+	fileassoc_cleanup_cb_t cb;
+	void *data;
+
+	cb = assoc->cleanup_cb;
+	if (cb == NULL) {
+		return;
+	}
+	data = table_getdata(tbl, assoc);
+	(*cb)(data, FILEASSOC_CLEANUP_TABLE);
+}
+
+static void *
+file_getdata(struct fileassoc_hash_entry *e, const struct fileassoc *assoc)
+{
+
+	return specificdata_getspecific(fileassoc_domain, &e->data,
+	    assoc->key);
+}
+
+static void
+file_setdata(struct fileassoc_hash_entry *e, const struct fileassoc *assoc,
+    void *data)
+{
+
+	specificdata_setspecific(fileassoc_domain, &e->data, assoc->key,
+	    data);
+}
+
+static void
+file_cleanup(struct fileassoc_hash_entry *e, const struct fileassoc *assoc)
+{
+	fileassoc_cleanup_cb_t cb;
+	void *data;
+
+	cb = assoc->cleanup_cb;
+	if (cb == NULL) {
+		return;
+	}
+	data = file_getdata(e, assoc);
+	(*cb)(data, FILEASSOC_CLEANUP_FILE);
+}
+
+static void
+file_free(struct fileassoc_hash_entry *e)
+{
+	struct fileassoc *assoc;
+
+	LIST_REMOVE(e, entries);
+
+	LIST_FOREACH(assoc, &fileassoc_list, list) {
+		file_cleanup(e, assoc);
+	}
+	vfs_composefh_free(e->handle);
+	specificdata_fini(fileassoc_domain, &e->data);
+	kmem_free(e, sizeof(*e));
+}
+
 /*
  * Initialize the fileassoc subsystem.
  */
-void
+static int
 fileassoc_init(void)
 {
-	memset(fileassoc_hooks, 0, sizeof(fileassoc_hooks));
-	fileassoc_nhooks = 0;
+
+	fileassoc_domain = specificdata_domain_create();
+
+	return 0;
 }
 
 /*
  * Register a new hook.
  */
-fileassoc_t
-fileassoc_register(const char *name, fileassoc_cleanup_cb_t cleanup_cb)
+int
+fileassoc_register(const char *name, fileassoc_cleanup_cb_t cleanup_cb,
+    fileassoc_t *result)
 {
-	int i;
-
-	if (fileassoc_nhooks >= FILEASSOC_NHOOKS)
-		return (-1);
-
-	for (i = 0; i < FILEASSOC_NHOOKS; i++)
-		if (fileassoc_hooks[i].hook_name == NULL)
-			break;
-
-	fileassoc_hooks[i].hook_name = name;
-	fileassoc_hooks[i].hook_cleanup_cb = cleanup_cb;
+	int error;
+	specificdata_key_t key;
+	struct fileassoc *assoc;
+	static ONCE_DECL(control);
 
-	fileassoc_nhooks++;
+	RUN_ONCE(&control, fileassoc_init);
+	error = specificdata_key_create(fileassoc_domain, &key, NULL);
+	if (error) {
+		return error;
+	}
+	assoc = kmem_alloc(sizeof(*assoc), KM_SLEEP);
+	assoc->name = name;
+	assoc->cleanup_cb = cleanup_cb;
+	assoc->key = key;
+	LIST_INSERT_HEAD(&fileassoc_list, assoc, list);
+	*result = assoc;
 
-	return (i);
+	return 0;
 }
 
 /*
  * Deregister a hook.
  */
 int
-fileassoc_deregister(fileassoc_t id)
+fileassoc_deregister(fileassoc_t assoc)
 {
-	if (id < 0 || id >= FILEASSOC_NHOOKS)
-		return (EINVAL);
-
-	fileassoc_hooks[id].hook_name = NULL;
-	fileassoc_hooks[id].hook_cleanup_cb = NULL;
 
-	fileassoc_nhooks--;
+	LIST_REMOVE(assoc, list);
+	kmem_free(assoc, sizeof(*assoc));
 
-	return (0);
+	return 0;
 }
 
 /*
@@ -222,7 +302,7 @@ fileassoc_file_lookup(struct vnode *vp, 
  * Return hook data associated with a vnode.
  */
 void *
-fileassoc_lookup(struct vnode *vp, fileassoc_t id)
+fileassoc_lookup(struct vnode *vp, fileassoc_t assoc)
 {
         struct fileassoc_hash_entry *mhe;
 
@@ -230,7 +310,7 @@ fileassoc_lookup(struct vnode *vp, filea
         if (mhe == NULL)
                 return (NULL);
 
-        return (mhe->hooks[id]);
+        return file_getdata(mhe, assoc);
 }
 
 /*
@@ -246,11 +326,12 @@ fileassoc_table_add(struct mount *mp, si
 		return (EEXIST);
 
 	/* Allocate and initialize a Veriexec hash table. */
-	tbl = malloc(sizeof(*tbl), M_TEMP, M_WAITOK | M_ZERO);
+	tbl = kmem_zalloc(sizeof(*tbl), KM_SLEEP);
 	tbl->hash_size = size;
 	tbl->tbl_mntpt = mp;
 	tbl->hash_tbl = hashinit(size, HASH_LIST, M_TEMP,
 				 M_WAITOK | M_ZERO, &tbl->hash_mask);
+	specificdata_init(fileassoc_domain, &tbl->data);
 
 	LIST_INSERT_HEAD(&fileassoc_tables, tbl, hash_list);
 
@@ -263,10 +344,10 @@ fileassoc_table_add(struct mount *mp, si
 int
 fileassoc_table_delete(struct mount *mp)
 {
+	const struct fileassoc *assoc;
 	struct fileassoc_table *tbl;
 	struct fileassoc_hashhead *hh;
 	u_long i;
-	int j;
 
 	tbl = fileassoc_table_lookup(mp);
 	if (tbl == NULL)
@@ -277,29 +358,20 @@ fileassoc_table_delete(struct mount *mp)
 	for (i = 0; i < tbl->hash_size; i++) {
 		struct fileassoc_hash_entry *mhe;
 
-		while (LIST_FIRST(&hh[i]) != NULL) {
-			mhe = LIST_FIRST(&hh[i]);
-			LIST_REMOVE(mhe, entries);
-
-			for (j = 0; j < fileassoc_nhooks; j++)
-				if (fileassoc_hooks[j].hook_cleanup_cb != NULL)
-					(fileassoc_hooks[j].hook_cleanup_cb)
-					    (mhe->hooks[j],
-					    FILEASSOC_CLEANUP_FILE);
-
-			vfs_composefh_free(mhe->handle);
-			free(mhe, M_TEMP);
+		while ((mhe = LIST_FIRST(&hh[i])) != NULL) {
+			file_free(mhe);
 		}
 	}
 
-	for (j = 0; j < fileassoc_nhooks; j++)
-		if (fileassoc_hooks[j].hook_cleanup_cb != NULL)
-			(fileassoc_hooks[j].hook_cleanup_cb)(tbl->tables[j],
-			    FILEASSOC_CLEANUP_TABLE);
+	LIST_FOREACH(assoc, &fileassoc_list, list) {
+		table_cleanup(tbl, assoc);
+	}
 
 	/* Remove hash table and sysctl node */
 	hashdone(tbl->hash_tbl, M_TEMP);
 	LIST_REMOVE(tbl, hash_list);
+	specificdata_fini(fileassoc_domain, &tbl->data);
+	kmem_free(tbl, sizeof(*tbl));
 
 	return (0);
 }
@@ -308,7 +380,7 @@ fileassoc_table_delete(struct mount *mp)
  * Run a callback for each hook entry in a table.
  */
 int
-fileassoc_table_run(struct mount *mp, fileassoc_t id, fileassoc_cb_t cb)
+fileassoc_table_run(struct mount *mp, fileassoc_t assoc, fileassoc_cb_t cb)
 {
 	struct fileassoc_table *tbl;
 	struct fileassoc_hashhead *hh;
@@ -323,8 +395,11 @@ fileassoc_table_run(struct mount *mp, fi
 		struct fileassoc_hash_entry *mhe;
 
 		LIST_FOREACH(mhe, &hh[i], entries) {
-			if (mhe->hooks[id] != NULL)
-				cb(mhe->hooks[id]);
+			void *data;
+
+			data = file_getdata(mhe, assoc);
+			if (data != NULL)
+				cb(data);
 		}
 	}
 
@@ -335,36 +410,28 @@ fileassoc_table_run(struct mount *mp, fi
  * Clear a table for a given hook.
  */
 int
-fileassoc_table_clear(struct mount *mp, fileassoc_t id)
+fileassoc_table_clear(struct mount *mp, fileassoc_t assoc)
 {
 	struct fileassoc_table *tbl;
 	struct fileassoc_hashhead *hh;
-	fileassoc_cleanup_cb_t cleanup_cb;
 	u_long i;
 
 	tbl = fileassoc_table_lookup(mp);
 	if (tbl == NULL)
 		return (EEXIST);
 
-	cleanup_cb = fileassoc_hooks[id].hook_cleanup_cb;
-
 	hh = tbl->hash_tbl;
 	for (i = 0; i < tbl->hash_size; i++) {
 		struct fileassoc_hash_entry *mhe;
 
 		LIST_FOREACH(mhe, &hh[i], entries) {
-			if ((mhe->hooks[id] != NULL) && cleanup_cb != NULL)
-				cleanup_cb(mhe->hooks[id],
-				    FILEASSOC_CLEANUP_FILE);
-
-			mhe->hooks[id] = NULL;
+			file_cleanup(mhe, assoc);
+			file_setdata(mhe, assoc, NULL);
 		}
 	}
 
-	if ((tbl->tables[id] != NULL) && cleanup_cb != NULL)
-		cleanup_cb(tbl->tables[id], FILEASSOC_CLEANUP_TABLE);
-
-	tbl->tables[id] = NULL;
+	table_cleanup(tbl, assoc);
+	table_setdata(tbl, assoc, NULL);
 
 	return (0);
 }
@@ -373,7 +440,7 @@ fileassoc_table_clear(struct mount *mp, 
  * Add hook-specific data on a fileassoc table.
  */
 int
-fileassoc_tabledata_add(struct mount *mp, fileassoc_t id, void *data)
+fileassoc_tabledata_add(struct mount *mp, fileassoc_t assoc, void *data)
 {
 	struct fileassoc_table *tbl;
 
@@ -381,7 +448,7 @@ fileassoc_tabledata_add(struct mount *mp
 	if (tbl == NULL)
 		return (EFAULT);
 
-	tbl->tables[id] = data;
+	table_setdata(tbl, assoc, data);
 
 	return (0);
 }
@@ -390,24 +457,17 @@ fileassoc_tabledata_add(struct mount *mp
  * Clear hook-specific data on a fileassoc table.
  */
 int
-fileassoc_tabledata_clear(struct mount *mp, fileassoc_t id)
+fileassoc_tabledata_clear(struct mount *mp, fileassoc_t assoc)
 {
-	struct fileassoc_table *tbl;
-
-	tbl = fileassoc_table_lookup(mp);
-	if (tbl == NULL)
-		return (EFAULT);
-
-	tbl->tables[id] = NULL;
 
-	return (0);
+	return fileassoc_tabledata_add(mp, assoc, NULL);
 }
 
 /*
  * Retrieve hook-specific data from a fileassoc table.
  */
 void *
-fileassoc_tabledata_lookup(struct mount *mp, fileassoc_t id)
+fileassoc_tabledata_lookup(struct mount *mp, fileassoc_t assoc)
 {
 	struct fileassoc_table *tbl;
 
@@ -415,7 +475,7 @@ fileassoc_tabledata_lookup(struct mount 
 	if (tbl == NULL)
 		return (NULL);
 
-	return (tbl->tables[id]);
+	return table_getdata(tbl, assoc);
 }
 
 /*
@@ -457,8 +517,9 @@ fileassoc_file_add(struct vnode *vp, fha
 	indx = FILEASSOC_HASH(tbl, th);
 	vhh = &(tbl->hash_tbl[indx]);
 
-	e = malloc(sizeof(*e), M_TEMP, M_WAITOK | M_ZERO);
+	e = kmem_zalloc(sizeof(*e), KM_SLEEP);
 	e->handle = th;
+	specificdata_init(fileassoc_domain, &e->data);
 	LIST_INSERT_HEAD(vhh, e, entries);
 
 	return (e);
@@ -471,20 +532,12 @@ int
 fileassoc_file_delete(struct vnode *vp)
 {
 	struct fileassoc_hash_entry *mhe;
-	int i;
 
 	mhe = fileassoc_file_lookup(vp, NULL);
 	if (mhe == NULL)
 		return (ENOENT);
 
-	LIST_REMOVE(mhe, entries);
-
-	for (i = 0; i < fileassoc_nhooks; i++)
-		if (fileassoc_hooks[i].hook_cleanup_cb != NULL)
-			(fileassoc_hooks[i].hook_cleanup_cb)(mhe->hooks[i],
-			    FILEASSOC_CLEANUP_FILE);
-
-	free(mhe, M_TEMP);
+	file_free(mhe);
 
 	return (0);
 }
@@ -493,9 +546,10 @@ fileassoc_file_delete(struct vnode *vp)
  * Add a hook to a vnode.
  */
 int
-fileassoc_add(struct vnode *vp, fileassoc_t id, void *data)
+fileassoc_add(struct vnode *vp, fileassoc_t assoc, void *data)
 {
 	struct fileassoc_hash_entry *e;
+	void *olddata;
 
 	e = fileassoc_file_lookup(vp, NULL);
 	if (e == NULL) {
@@ -504,10 +558,11 @@ fileassoc_add(struct vnode *vp, fileasso
 			return (ENOTDIR);
 	}
 
-	if (e->hooks[id] != NULL)
+	olddata = file_getdata(e, assoc);
+	if (olddata != NULL)
 		return (EEXIST);
 
-	e->hooks[id] = data;
+	file_setdata(e, assoc, data);
 
 	return (0);
 }
@@ -516,20 +571,16 @@ fileassoc_add(struct vnode *vp, fileasso
  * Clear a hook from a vnode.
  */
 int
-fileassoc_clear(struct vnode *vp, fileassoc_t id)
+fileassoc_clear(struct vnode *vp, fileassoc_t assoc)
 {
 	struct fileassoc_hash_entry *mhe;
-	fileassoc_cleanup_cb_t cleanup_cb;
 
 	mhe = fileassoc_file_lookup(vp, NULL);
 	if (mhe == NULL)
 		return (ENOENT);
 
-	cleanup_cb = fileassoc_hooks[id].hook_cleanup_cb;
-	if ((mhe->hooks[id] != NULL) && cleanup_cb != NULL)
-		cleanup_cb(mhe->hooks[id], FILEASSOC_CLEANUP_FILE);
-
-	mhe->hooks[id] = NULL;
+	file_cleanup(mhe, assoc);
+	file_setdata(mhe, assoc, NULL);
 
 	return (0);
 }
Index: kern/kern_verifiedexec.c
===================================================================
--- kern/kern_verifiedexec.c	(revision 1926)
+++ kern/kern_verifiedexec.c	(working copy)
@@ -102,7 +102,7 @@ size_t veriexec_name_max;
 
 const struct sysctlnode *veriexec_count_node;
 
-int veriexec_hook;
+static fileassoc_t veriexec_hook;
 
 LIST_HEAD(, veriexec_fpops) veriexec_fpops_list;
 
@@ -267,10 +267,12 @@ veriexec_fpops_add(const char *fp_type, 
 void
 veriexec_init(void)
 {
+	int error;
+
 	/* Register a fileassoc for Veriexec. */
-	veriexec_hook = fileassoc_register("veriexec", veriexec_clear);
-	if (veriexec_hook == FILEASSOC_INVAL)
-		panic("Veriexec: Can't register fileassoc");
+	error = fileassoc_register("veriexec", veriexec_clear, &veriexec_hook);
+	if (error != 0)
+		panic("Veriexec: Can't register fileassoc: error=%d", error);
 
 	/* Register listener to handle raw disk access. */
 	if (kauth_listen_scope(KAUTH_SCOPE_DEVICE, veriexec_raw_cb, NULL) ==
Index: kern/kern_pax.c
===================================================================
--- kern/kern_pax.c	(revision 1922)
+++ kern/kern_pax.c	(working copy)
@@ -182,12 +182,20 @@ SYSCTL_SETUP(sysctl_security_pax_setup, 
 void
 pax_init(void)
 {
+#ifdef PAX_SEGVGUARD
+	int error;
+#endif /* PAX_SEGVGUARD */
+
 #ifdef PAX_MPROTECT
 	proc_specific_key_create(&pax_mprotect_key, NULL);
 #endif /* PAX_MPROTECT */
 
 #ifdef PAX_SEGVGUARD
-	segvguard_id = fileassoc_register("segvguard", pax_segvguard_cb);
+	error = fileassoc_register("segvguard", pax_segvguard_cb,
+	    &segvguard_id);
+	if (error) {
+		panic("pax_init: segvguard_id: error=%d\n", error);
+	}
 	proc_specific_key_create(&pax_segvguard_key, NULL);
 #endif /* PAX_SEGVGUARD */
 }
@@ -286,7 +294,7 @@ pax_segvguard(struct lwp *l, struct vnod
 	    (!pax_segvguard_global && t != PAX_SEGVGUARD_EXPLICIT_ENABLE))
 		return (0);
 
-	if (segvguard_id == FILEASSOC_INVAL || vp == NULL)
+	if (vp == NULL)
 		return (EFAULT);	
 
 	/* Check if we already monitor the file. */

--NextPart-20061208225838-1213100--