Some SELinux patches for 2.6.26

While looking at the SELinux secid/secctx conversion functions I realized they could probably do with a little cleanup to reduce the amount of code and make better use of existing string processing functions in the kernel. Making use of the kernel's existing string processing functions is a good idea as many architectures have specialized/optimized routines which should be an improvement over the generic code in the SELinux security server.

Signed-off-by Paul Moore <[email protected]>

---

 security/selinux/ss/mls.c      |   61 +++++--------
 security/selinux/ss/mls.h      |    3 -
 security/selinux/ss/services.c |  188 ++++++++++++++++------------------------
 3 files changed, 99 insertions(+), 153 deletions(-)

security/selinux/ss/mls.c +23 -38

index feaf0a5..ba211b4 100644
@@ -239,8 +239,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
  * Policy read-lock must be held for sidtab lookup.
  *
  */
-int mls_context_to_sid(char oldc,
-		       char **scontext,
+int mls_context_to_sid(char **scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid)

@@ -250,10 +249,10 @@ int mls_context_to_sid(char oldc,
 	char *scontextp, *p, *rngptr;
 	struct level_datum *levdatum;
 	struct cat_datum *catdatum, *rngdatum;
-	int l, rc = -EINVAL;
+	int l, rc;
 
 	if (!selinux_mls_enabled) {
-		if (def_sid != SECSID_NULL && oldc)
+		if (def_sid != SECSID_NULL && *scontext && **scontext)
 			*scontext += strlen(*scontext)+1;
 		return 0;
 	}

@@ -262,18 +261,17 @@ int mls_context_to_sid(char oldc,
 	 * No MLS component to the security context, try and map to
 	 * default if provided.
 	 */
-	if (!oldc) {
+	if (!(*scontext) || !(**scontext)) {
 		struct context *defcon;
 
 		if (def_sid == SECSID_NULL)
-			goto out;
+			return -EINVAL;
 
 		defcon = sidtab_search(s, def_sid);
 		if (!defcon)
-			goto out;
+			return -EINVAL;
 
-		rc = mls_context_cpy(context, defcon);
-		goto out;
+		return mls_context_cpy(context, defcon);
 	}
 
 	/* Extract low sensitivity. */

@@ -287,10 +285,8 @@ int mls_context_to_sid(char oldc,
 
 	for (l = 0; l < 2; l++) {
 		levdatum = hashtab_search(policydb.p_levels.table, scontextp);
-		if (!levdatum) {
-			rc = -EINVAL;
-			goto out;
-		}
+		if (!levdatum)
+			return -EINVAL;
 
 		context->range.level[l].sens = levdatum->level->sens;
 

@@ -312,35 +308,29 @@ int mls_context_to_sid(char oldc,
 
 				catdatum = hashtab_search(policydb.p_cats.table,
 				                          scontextp);
-				if (!catdatum) {
-					rc = -EINVAL;
-					goto out;
-				}
+				if (!catdatum)
+					return -EINVAL;
 
 				rc = ebitmap_set_bit(&context->range.level[l].cat,
 				                     catdatum->value - 1, 1);
 				if (rc)
-					goto out;
+					return rc;
 
 				/* If range, set all categories in range */
 				if (rngptr) {
 					int i;
 
 					rngdatum = hashtab_search(policydb.p_cats.table, rngptr);
-					if (!rngdatum) {
-						rc = -EINVAL;
-						goto out;
-					}
+					if (!rngdatum)
+						return -EINVAL;
 
-					if (catdatum->value >= rngdatum->value) {
-						rc = -EINVAL;
-						goto out;
-					}
+					if (catdatum->value >= rngdatum->value)
+						return -EINVAL;
 
 					for (i = catdatum->value; i < rngdatum->value; i++) {
 						rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
 						if (rc)
-							goto out;
+							return rc;
 					}
 				}
 

@@ -366,12 +356,10 @@ int mls_context_to_sid(char oldc,
 		rc = ebitmap_cpy(&context->range.level[1].cat,
 				 &context->range.level[0].cat);
 		if (rc)
-			goto out;
+			return rc;
 	}
 	*scontext = ++p;
-	rc = 0;
-out:
-	return rc;
+	return 0;
 }
 
 /*

@@ -391,13 +379,10 @@ int mls_from_string(char *str, struct context *context, gfp_t gfp_mask)
 	/* we need freestr because mls_context_to_sid will change
 	   the value of tmpstr */
 	tmpstr = freestr = kstrdup(str, gfp_mask);
-	if (!tmpstr) {
-		rc = -ENOMEM;
-	} else {
-		rc = mls_context_to_sid(':', &tmpstr, context,
-		                        NULL, SECSID_NULL);
-		kfree(freestr);
-	}
+	if (!tmpstr)
+		return -ENOMEM;
+	rc = mls_context_to_sid(&tmpstr, context, NULL, SECSID_NULL);
+	kfree(freestr);
 
 	return rc;
 }

security/selinux/ss/mls.h +1 -2

index ab53663..b364f61 100644
@@ -30,8 +30,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c);
 int mls_range_isvalid(struct policydb *p, struct mls_range *r);
 int mls_level_isvalid(struct policydb *p, struct mls_level *l);
 
-int mls_context_to_sid(char oldc,
-	               char **scontext,
+int mls_context_to_sid(char **scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid);

security/selinux/ss/services.c +75 -114

index f374186..c9943ca 100644
@@ -573,47 +573,42 @@ out:
 	return rc;
 }
 
-/*
- * Write the security context string representation of
- * the context structure `context' into a dynamically
- * allocated string of the correct size.  Set `*scontext'
- * to point to this string and set `*scontext_len' to
- * the length of the string.
+/**
+ * context_struct_to_string - Generate a string representation of a context
+ * @context: native security context
+ * @scontext: security context string
+ * @scontext_len: length of security context string
+ *
+ * Description:
+ * Write the security context string representation of @context into
+ * @scontext.  The caller must free @scontext when finished.  Returns zero on
+ * success, negative values on failure.
+ *
  */
 static int context_struct_to_string(struct context *context, char **scontext, u32 *scontext_len)
 {
-	char *scontextp;
+	char *ctx;
 
-	*scontext = NULL;
-	*scontext_len = 0;
-
-	/* Compute the size of the context. */
-	*scontext_len += strlen(policydb.p_user_val_to_name[context->user - 1]) + 1;
-	*scontext_len += strlen(policydb.p_role_val_to_name[context->role - 1]) + 1;
-	*scontext_len += strlen(policydb.p_type_val_to_name[context->type - 1]) + 1;
-	*scontext_len += mls_compute_context_len(context);
+	*scontext_len = strlen(policydb.p_user_val_to_name[context->user - 1]) +
+		strlen(policydb.p_role_val_to_name[context->role - 1]) +
+		strlen(policydb.p_type_val_to_name[context->type - 1]) +
+		mls_compute_context_len(context) + 3;
 
 	/* Allocate space for the context; caller must free this space. */
-	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
-	if (!scontextp) {
+	*scontext = kmalloc(*scontext_len, GFP_ATOMIC);
+	if (!*scontext) {
+		*scontext_len = 0;
 		return -ENOMEM;
 	}
-	*scontext = scontextp;
 
-	/*
-	 * Copy the user name, role name and type name into the context.
-	 */
-	sprintf(scontextp, "%s:%s:%s",
-		policydb.p_user_val_to_name[context->user - 1],
-		policydb.p_role_val_to_name[context->role - 1],
-		policydb.p_type_val_to_name[context->type - 1]);
-	scontextp += strlen(policydb.p_user_val_to_name[context->user - 1]) +
-	             1 + strlen(policydb.p_role_val_to_name[context->role - 1]) +
-	             1 + strlen(policydb.p_type_val_to_name[context->type - 1]);
-
-	mls_sid_to_context(context, &scontextp);
-
-	*scontextp = 0;
+	ctx = *scontext;
+	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
+	strcat(ctx, ":");
+	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
+	strcat(ctx, ":");
+	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);
+	ctx += strlen(ctx);
+	mls_sid_to_context(context, &ctx);
 
 	return 0;
 }

@@ -640,68 +635,61 @@ const char *security_get_initial_sid_context(u32 sid)
 int security_sid_to_context(u32 sid, char **scontext, u32 *scontext_len)
 {
 	struct context *context;
-	int rc = 0;
-
-	*scontext = NULL;
-	*scontext_len  = 0;
+	int rc;
 
 	if (!ss_initialized) {
-		if (sid <= SECINITSID_NUM) {
-			char *scontextp;
-
-			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
-			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
-			if (!scontextp) {
-				rc = -ENOMEM;
-				goto out;
-			}
-			strcpy(scontextp, initial_sid_to_string[sid]);
-			*scontext = scontextp;
-			goto out;
+		if (sid > SECINITSID_NUM) {
+			printk(KERN_ERR
+			       "security_sid_to_context:  called before "
+			       "initial load_policy on unknown SID %d\n", sid);
+			goto err;
 		}
-		printk(KERN_ERR "security_sid_to_context:  called before initial "
-		       "load_policy on unknown SID %d\n", sid);
-		rc = -EINVAL;
-		goto out;
+		*scontext = kstrdup(initial_sid_to_string[sid], GFP_ATOMIC);
+		if (*scontext == NULL)
+			return -ENOMEM;
+		*scontext_len = strlen(*scontext) + 1;
+		return 0;
 	}
+
 	POLICY_RDLOCK;
 	context = sidtab_search(&sidtab, sid);
 	if (!context) {
-		printk(KERN_ERR "security_sid_to_context:  unrecognized SID "
-		       "%d\n", sid);
-		rc = -EINVAL;
-		goto out_unlock;
+		POLICY_RDUNLOCK;
+		printk(KERN_ERR
+		       "security_sid_to_context:  unrecognized SID %d\n", sid);
+		goto err;
 	}
 	rc = context_struct_to_string(context, scontext, scontext_len);
-out_unlock:
 	POLICY_RDUNLOCK;
-out:
 	return rc;
 
+err:
+	*scontext = NULL;
+	*scontext_len = 0;
+	return -EINVAL;
 }
 
 static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *sid, u32 def_sid)
 {
-	char *scontext2;
+	char *scontext_dup, *scontext_iter, *scontext_next;
 	struct context context;
-	struct role_datum *role;
+	struct role_datum *roledatum;
 	struct type_datum *typdatum;
 	struct user_datum *usrdatum;
-	char *scontextp, *p, oldc;
-	int rc = 0;
+	int rc = -EINVAL;
 
 	if (!ss_initialized) {
 		int i;
 
-		for (i = 1; i < SECINITSID_NUM; i++) {
+		for (i = 1; i < SECINITSID_NUM; i++)
 			if (!strcmp(initial_sid_to_string[i], scontext)) {
 				*sid = i;
-				goto out;
+				return 0;
 			}
-		}
 		*sid = SECINITSID_KERNEL;
-		goto out;
+		return 0;
 	}
+
 	*sid = SECSID_NULL;
 
 	/* Copy the string so that we can modify the copy as we parse it.

@@ -709,73 +697,46 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 	   null suffix to the copy to avoid problems with the existing
 	   attr package, which doesn't view the null terminator as part
 	   of the attribute value. */
-	scontext2 = kmalloc(scontext_len+1,GFP_KERNEL);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;
+	scontext_dup = kmemdup(scontext, scontext_len + 1, GFP_KERNEL);
+	if (!scontext_dup)
+		return -ENOMEM;
+	scontext_dup[scontext_len] = 0;
 
 	context_init(&context);
-	*sid = SECSID_NULL;
+	scontext_next = scontext_dup;
 
 	POLICY_RDLOCK;
 
-	/* Parse the security context. */
-
-	rc = -EINVAL;
-	scontextp = (char *) scontext2;
-
 	/* Extract the user. */
-	p = scontextp;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	usrdatum = hashtab_search(policydb.p_users.table, scontextp);
+	usrdatum = hashtab_search(policydb.p_users.table, scontext_iter);
 	if (!usrdatum)
 		goto out_unlock;
-
 	context.user = usrdatum->value;
 
 	/* Extract role. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	role = hashtab_search(policydb.p_roles.table, scontextp);
-	if (!role)
+	roledatum = hashtab_search(policydb.p_roles.table, scontext_iter);
+	if (!roledatum)
 		goto out_unlock;
-	context.role = role->value;
+	context.role = roledatum->value;
 
 	/* Extract type. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-	oldc = *p;
-	*p++ = 0;
-
-	typdatum = hashtab_search(policydb.p_types.table, scontextp);
+	scontext_iter = strsep(&scontext_next, ":");
+	typdatum = hashtab_search(policydb.p_types.table, scontext_iter);
 	if (!typdatum)
 		goto out_unlock;
-
 	context.type = typdatum->value;
 
-	rc = mls_context_to_sid(oldc, &p, &context, &sidtab, def_sid);
+	/* Extract MLS range. */
+	rc = mls_context_to_sid(&scontext_next, &context, &sidtab, def_sid);
 	if (rc)
 		goto out_unlock;
-
-	if ((p - scontext2) < scontext_len) {
+	if (scontext_next - scontext_dup < scontext_len) {
 		rc = -EINVAL;
 		goto out_unlock;
 	}

@@ -785,13 +746,14 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 		rc = -EINVAL;
 		goto out_unlock;
 	}
+
 	/* Obtain the new sid. */
 	rc = sidtab_context_to_sid(&sidtab, &context, sid);
+
 out_unlock:
 	POLICY_RDUNLOCK;
 	context_destroy(&context);
-	kfree(scontext2);
-out:
+	kfree(scontext_dup);
 	return rc;
 }
 
 
 
--

This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

While looking at the SELinux secid/secctx conversion functions I realized they could probably do with a little cleanup to reduce the amount of code and make better use of existing string processing functions in the kernel. Making use of the kernel's existing string processing functions is a good idea as many architectures have specialized/optimized routines which should be an improvement over the generic code in the SELinux security server.

Signed-off-by Paul Moore <[email protected]>

---

 security/selinux/ss/mls.c      |   61 +++++--------
 security/selinux/ss/mls.h      |    3 -
 security/selinux/ss/services.c |  188 ++++++++++++++++------------------------
 3 files changed, 99 insertions(+), 153 deletions(-)

security/selinux/ss/services.c +0 -0

@@ -709,73 +697,46 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 	   null suffix to the copy to avoid problems with the existing

security/selinux/ss/services.c +17 -38

+	if (!scontext_dup)
+		return -ENOMEM;
+	scontext_dup[scontext_len] = 0;
 
 
 	context_init(&context);
-	*sid = SECSID_NULL;
+	scontext_next = scontext_dup;
 
 	POLICY_RDLOCK;
 
-	/* Parse the security context. */
-
-	rc = -EINVAL;
-	scontextp = (char *) scontext2;
-
 	/* Extract the user. */
-	p = scontextp;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	usrdatum = hashtab_search(policydb.p_users.table, scontextp);
+	usrdatum = hashtab_search(policydb.p_users.table, scontext_iter);
 	if (!usrdatum)
 		goto out_unlock;
-
 	context.user = usrdatum->value;
 
 	/* Extract role. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	role = hashtab_search(policydb.p_roles.table, scontextp);
-	if (!role)
+	roledatum = hashtab_search(policydb.p_roles.table, scontext_iter);
+	if (!roledatum)
 		goto out_unlock;
-	context.role = role->value;
+	context.role = roledatum->value;
 
 	/* Extract type. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-	oldc = *p;
-	*p++ = 0;
-
-	typdatum = hashtab_search(policydb.p_types.table, scontextp);
+	scontext_iter = strsep(&scontext_next, ":");
+	typdatum = hashtab_search(policydb.p_types.table, scontext_iter);
 	if (!typdatum)
 		goto out_unlock;
-
 	context.type = typdatum->value;
 
-	rc = mls_context_to_sid(oldc, &p, &context, &sidtab, def_sid);
+	/* Extract MLS range. */
+	rc = mls_context_to_sid(&scontext_next, &context, &sidtab, def_sid);
 	if (rc)
 		goto out_unlock;
-
-	if ((p - scontext2) < scontext_len) {
+	if (scontext_next - scontext_dup < scontext_len) {
 		rc = -EINVAL;
 		goto out_unlock;

security/selinux/ss/services.c +3 -3

 	}

@@ -785,13 +746,14 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 		rc = -EINVAL;
 		goto out_unlock;
 	}
+
 	/* Obtain the new sid. */
 	rc = sidtab_context_to_sid(&sidtab, &context, sid);
+
 out_unlock:
 	POLICY_RDUNLOCK;
 	context_destroy(&context);
-	kfree(scontext2);
-out:
+	kfree(scontext_dup);
 	return rc;
 }
 
 
 
--

This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Quoted 11 lines · Paul Moore

(quoted diff) +0 -0

@@ -709,73 +697,46 @@ static int security_context_to_sid_core(char
 *scontext, u32 scontext_len, u32 *s null suffix to the copy to

avoid problems with the existing attr package, which doesn't view the null terminator as part of the attribute value. */

security/selinux/ss/services.c +0 -7

-	scontext2 = kmalloc(scontext_len+1,GFP_KERNEL);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;

security/selinux/ss/services.c static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s · line 700

Quoted 1 line

security/selinux/ss/services.c +1 -0

+	scontext_dup = kmemdup(scontext, scontext_len + 1, GFP_KERNEL);

Also, in addition to the gfp_flags change, I'm not clear that the above change is correct. We are taking a byte array "scontext" of length "scontext_len" and copying it into a buffer of length "scontext_len+1" so that we can ensure that it is NUL terminated prior to parsing. Won't kmemdup with scontext_len+1 ultimately run off the end of the original string?

Quoted 12 lines · Stephen Smalley

Quoted 11 lines · Paul Moore

(quoted diff) +0 -0

@@ -709,73 +697,46 @@ static int security_context_to_sid_core(char
 *scontext, u32 scontext_len, u32 *s null suffix to the copy to

avoid problems with the existing attr package, which doesn't view the null terminator as part of the attribute value. */

security/selinux/ss/services.c +0 -7

-	scontext2 = kmalloc(scontext_len+1,GFP_KERNEL);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;

Quoted 8 lines

security/selinux/ss/services.c static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s · line 700

Quoted 1 line

security/selinux/ss/services.c +1 -0

+	scontext_dup = kmemdup(scontext, scontext_len + 1, GFP_KERNEL);

Also, in addition to the gfp_flags change, I'm not clear that the above change is correct. We are taking a byte array "scontext" of length "scontext_len" and copying it into a buffer of length "scontext_len+1" so that we can ensure that it is NUL terminated prior to parsing. Won't kmemdup with scontext_len+1 ultimately run off the end of the original string?

Good point, I believe you're right. I'll add this and the gfp stuff to the list of needed changes.

I think I may also suggest shelving this patch for 2.6.26 as a little birdie mentioned it would be a good idea to give this a through testing on non-MLS/MCS systems which I haven't yet done and don't expect to be able to do so before the merge window opens.

When adding new entries to the network node cache we would walk the entire hash bucket to make sure we didn't cross a threshold (done to bound the cache size). This isn't a very quick or elegant solution for something which is supposed to be quick-ish so add a counter to each hash bucket to track the size of the bucket and eliminate the need to walk the entire bucket list on each add.

Signed-off-by Paul Moore <[email protected]>

---

 security/selinux/netnode.c |   57 +++++++++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 30 deletions(-)

security/selinux/netnode.c +0 -0

index f3c526f..b3a5a65 100644
@@ -40,11 +40,17 @@
 #include <net/ipv6.h>

security/selinux/netnode.c +3 -2

+	struct list_head list;
+};
+
 struct sel_netnode {
 	struct netnode_security_struct nsec;
 

@@ -179,35 +183,22 @@ static int sel_netnode_insert(struct sel_netnode *node)
 	default:
 		BUG();
 	}
-	list_add_rcu(&node->list, &sel_netnode_hash[idx]);
 
 	/* we need to impose a limit on the growth of the hash table so check
 	 * this bucket to make sure it is within the specified bounds */
-	list_for_each_entry(iter, &sel_netnode_hash[idx], list)

security/selinux/netnode.c +3 -0

+		call_rcu(&tail->rcu, sel_netnode_free);
+	} else
+		sel_netnode_hash[idx].size++;
 
 	return 0;
 }

Quoted 25 lines · Paul Moore

When adding new entries to the network node cache we would walk the entire hash bucket to make sure we didn't cross a threshold (done to bound the cache size). This isn't a very quick or elegant solution for something which is supposed to be quick-ish so add a counter to each hash bucket to track the size of the bucket and eliminate the need to walk the entire bucket list on each add.

Signed-off-by Paul Moore <[email protected]>

---

 security/selinux/netnode.c |   57

+++++++++++++++++++++----------------------- 1 files changed, 27 insertions(+), 30 deletions(-)

(quoted diff) +0 -0

b/security/selinux/netnode.c index f3c526f..b3a5a65 100644

security/selinux/netnode.c +1 -0

@@ -40,11 +40,17 @@
 #include <net/ipv6.h>
 #include <asm/bug.h>
 
+#include "netnode.h"
 #include "objsec.h"
 

security/selinux/netnode.c line 50

Quoted 5 lines

security/selinux/netnode.c +2 -0

 #define SEL_NETNODE_HASH_SIZE       256
 #define SEL_NETNODE_HASH_BKT_LIMIT   16
 
+struct sel_netnode_bkt {
+	u32 size;

Technically this could just be an ordinary unsigned int, right? We tend to overuse fixed-size integers in our code at present.

Quoted 2 lines

security/selinux/netnode.c +2 -0

+	struct list_head list;
+};

Quoted 6 lines

security/selinux/netnode.c +0 -0

@@ -179,35 +183,22 @@ static int sel_netnode_insert(struct

sel_netnode *node) default:

security/selinux/netnode.c +0 -1

 		BUG();
 	}
-	list_add_rcu(&node->list, &sel_netnode_hash[idx]);

security/selinux/netnode.c static int sel_netnode_insert(struct sel_netnode *node) · line 193

Quoted 13 lines

/* we need to impose a limit on the growth of the hash table so
check * this bucket to make sure it is within the specified bounds
*/ - list_for_each_entry(iter, &sel_netnode_hash[idx], list)

security/selinux/netnode.c +5 -5

-		if (++count > SEL_NETNODE_HASH_BKT_LIMIT) {
-			list_del_rcu(&iter->list);
-			call_rcu(&iter->rcu, sel_netnode_free);
-			break;
-		}
+	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
+	if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) {
+		struct sel_netnode *tail;
+		tail = list_entry(node->list.prev, struct sel_netnode, list);
+		__list_del(node->list.prev, node->list.next);

Can you clarify the change from list_del_rcu() to __list_del() here?

Quoted 6 lines

security/selinux/netnode.c +3 -0

+		call_rcu(&tail->rcu, sel_netnode_free);
+	} else
+		sel_netnode_hash[idx].size++;
 
 	return 0;
 }

Quoted 26 lines · Stephen Smalley

Quoted 25 lines · Paul Moore

When adding new entries to the network node cache we would walk the entire hash bucket to make sure we didn't cross a threshold (done to bound the cache size). This isn't a very quick or elegant solution for something which is supposed to be quick-ish so add a counter to each hash bucket to track the size of the bucket and eliminate the need to walk the entire bucket list on each add.

Signed-off-by Paul Moore <[email protected]>

---

 security/selinux/netnode.c |   57

+++++++++++++++++++++----------------------- 1 files changed, 27 insertions(+), 30 deletions(-)

(quoted diff) +0 -0

b/security/selinux/netnode.c index f3c526f..b3a5a65 100644

security/selinux/netnode.c +1 -0

@@ -40,11 +40,17 @@
 #include <net/ipv6.h>
 #include <asm/bug.h>
 
+#include "netnode.h"
 #include "objsec.h"
 

Quoted 8 lines

security/selinux/netnode.c line 50

Quoted 5 lines

security/selinux/netnode.c +2 -0

 #define SEL_NETNODE_HASH_SIZE       256
 #define SEL_NETNODE_HASH_BKT_LIMIT   16
 
+struct sel_netnode_bkt {
+	u32 size;

Technically this could just be an ordinary unsigned int, right? We tend to overuse fixed-size integers in our code at present.

[Thanks for taking a look]

Yep, we can make this anything we want. I suppose if we are adverse to fixed-size scalars (not really an issue here, but I understand your point) a size_t variable might be a better pick since it is a "size" even though it isn't the traditional memory chunk size. The only drawback is that it varies in size, but it shouldn't messup the packing of the struct.

I guess I still kinda like the limit imposed by the u32 (I suppose I could make it smaller, u8?, but the compiler is just going to waste the memory for alignment reasons anyway) but if you feel strongly about this I can change it to an int or size_t type.

Quoted 3 lines

Quoted 3 lines

security/selinux/netnode.c +2 -0

+	struct list_head list;
+};

...

Quoted 6 lines

Quoted 6 lines

security/selinux/netnode.c +0 -0

@@ -179,35 +183,22 @@ static int sel_netnode_insert(struct

sel_netnode *node) default:

security/selinux/netnode.c +0 -1

 		BUG();
 	}
-	list_add_rcu(&node->list, &sel_netnode_hash[idx]);

Quoted 15 lines

security/selinux/netnode.c static int sel_netnode_insert(struct sel_netnode *node) · line 193

Quoted 13 lines

/* we need to impose a limit on the growth of the hash table so
check * this bucket to make sure it is within the specified bounds
*/ - list_for_each_entry(iter, &sel_netnode_hash[idx], list)

security/selinux/netnode.c +5 -5

-		if (++count > SEL_NETNODE_HASH_BKT_LIMIT) {
-			list_del_rcu(&iter->list);
-			call_rcu(&iter->rcu, sel_netnode_free);
-			break;
-		}
+	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
+	if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) {
+		struct sel_netnode *tail;
+		tail = list_entry(node->list.prev, struct sel_netnode, list);
+		__list_del(node->list.prev, node->list.next);

Can you clarify the change from list_del_rcu() to __list_del() here?

Well, the main reason for the change is I want to delete the tail of the list without having to traverse the entire list, there doesn't appear to be a list_*_rcu() that will allow me to do this so I simply used the __list_del() function to do what I needed. The only danger here is maintaining all of the right RCUisms and looking at the different implementations the only real difference is that the RCU version sets the removed entry's prev pointer to a poison value. Since
I "immediately" remove the entry and don't make use of the prev pointer in any of the list functions I decided to just skip that step.

However, I am not an RCU expert (tricky thing to master) so if this sounds problematic to you let me know and I'll add the "poisoning" to the code.

Quoted 31 lines · Paul Moore

Quoted 12 lines · Stephen Smalley

Quoted 11 lines · Paul Moore

(quoted diff) +0 -0

b/security/selinux/netnode.c index f3c526f..b3a5a65 100644

security/selinux/netnode.c +1 -0

@@ -40,11 +40,17 @@
 #include <net/ipv6.h>
 #include <asm/bug.h>
 
+#include "netnode.h"
 #include "objsec.h"
 

Quoted 8 lines

security/selinux/netnode.c line 50

Quoted 5 lines

security/selinux/netnode.c +2 -0

 #define SEL_NETNODE_HASH_SIZE       256
 #define SEL_NETNODE_HASH_BKT_LIMIT   16
 
+struct sel_netnode_bkt {
+	u32 size;

Technically this could just be an ordinary unsigned int, right? We tend to overuse fixed-size integers in our code at present.

[Thanks for taking a look]

Yep, we can make this anything we want. I suppose if we are adverse to fixed-size scalars (not really an issue here, but I understand your point) a size_t variable might be a better pick since it is a "size" even though it isn't the traditional memory chunk size. The only drawback is that it varies in size, but it shouldn't messup the packing of the struct.

Quoted 4 lines

I guess I still kinda like the limit imposed by the u32 (I suppose I could make it smaller, u8?, but the compiler is just going to waste the memory for alignment reasons anyway) but if you feel strongly about this I can change it to an int or size_t type.

Just pointing out that we tend to use fixed-size integers too widely, when an ordinary unsigned int or unsigned long would suffice. We only generally need the fixed size integers for file (e.g. policy image) and network packet formats. Not new to this code, but something to be aware of.

Quoted 34 lines

Quoted 6 lines

Quoted 6 lines

security/selinux/netnode.c +0 -0

@@ -179,35 +183,22 @@ static int sel_netnode_insert(struct

sel_netnode *node) default:

security/selinux/netnode.c +0 -1

 		BUG();
 	}
-	list_add_rcu(&node->list, &sel_netnode_hash[idx]);

Quoted 16 lines

Quoted 12 lines

/* we need to impose a limit on the growth of the hash table
so check * this bucket to make sure it is within the specified
bounds */ - list_for_each_entry(iter, &sel_netnode_hash[idx],
list) - if (++count > SEL_NETNODE_HASH_BKT_LIMIT) {

security/selinux/netnode.c +2 -4

-			list_del_rcu(&iter->list);
-			call_rcu(&iter->rcu, sel_netnode_free);
-			break;
-		}
+	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
+	if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT)

{ + struct sel_netnode *tail;

security/selinux/netnode.c +1 -0

+		tail = list_entry(node->list.prev, struct sel_netnode,

Quoted 1 line

list); + __list_del(node->list.prev, node->list.next);

Can you clarify the change from list_del_rcu() to __list_del() here?

Well, the main reason for the change is I want to delete the tail of the list without having to traverse the entire list, there doesn't appear to be a list_*_rcu() that will allow me to do this so I simply used the __list_del() function to do what I needed. The only danger here is maintaining all of the right RCUisms and looking at the different implementations the only real difference is that the RCU version sets the removed entry's prev pointer to a poison value. Since I "immediately" remove the entry and don't make use of the prev pointer in any of the list functions I decided to just skip that step.

Quoted 3 lines

However, I am not an RCU expert (tricky thing to master) so if this sounds problematic to you let me know and I'll add the "poisoning" to the code.

I don't quite understand. {snip}

Much like we added a network node cache, this patch adds a network port cache. The design is taken almost completely from the network node cache which in turn was taken from the network interface cache. The basic idea is to cache entries in a hash table based on protocol/port information. The hash function only takes the port number into account since the number of different protocols in use at any one time is expected to be relatively small.

Quoted 7 lines · Paul Moore

Much like we added a network node cache, this patch adds a network port cache. The design is taken almost completely from the network node cache which in turn was taken from the network interface cache. The basic idea is to cache entries in a hash table based on protocol/port information. The hash function only takes the port number into account since the number of different protocols in use at any one time is expected to be relatively small.

Not necessarily an obstacle to merging, but I was wondering if it would be worthwhile to investigating unifying these caches into a single code base with a more general interface, or if that would be a loss overall.

Quoted 9 lines · Stephen Smalley

Quoted 7 lines · Paul Moore

Much like we added a network node cache, this patch adds a network port cache. The design is taken almost completely from the network node cache which in turn was taken from the network interface cache. The basic idea is to cache entries in a hash table based on protocol/port information. The hash function only takes the port number into account since the number of different protocols in use at any one time is expected to be relatively small.

Quoted 4 lines

Not necessarily an obstacle to merging, but I was wondering if it would be worthwhile to investigating unifying these caches into a single code base with a more general interface, or if that would be a loss overall.

Yeah, I thought of that too when I was doing the network node cache but I convinced myself it would probably do more harm than good. The core problem is that all of the hashing and lookup functions are slightly different. Not fatal, we can workaround it via case statments, a function table, or something similar but all of these solutions will impose some additional overhead. For something which is supposed to be fast I thought it better to not go down this path.

I'm open to suggestions if you have any.