diff mbox

libsepol: In module_to_cil create one attribute for each unique set

Message ID 1490722110-31849-1-git-send-email-jwcart2@tycho.nsa.gov (mailing list archive)
State Not Applicable
Headers show

Commit Message

James Carter March 28, 2017, 5:28 p.m. UTC
CIL does not allow type or role sets in certain rules (such as allow
rules). It does, however, allow sets in typeattributeset and
roleattributeset statements. Because of this, when module_to_cil
translates a policy into CIL, it creates a new attribute for each
set that it encounters. But often the same set is used multiple times
which means that more attributes are created then necessary. As the
number of attributes increases the time required for the kernel to
make each policy decision increases which can be a problem.

To help reduce the number of attributes in a kernel policy,
when module_to_cil encounters a role or type set search to see if the
set was encountered already and, if it was, use the previously
generated attribute instead of creating a new one.

Testing on Android and Refpolicy policies show that this reduces the
number of attributes generated by about 40%.

Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
---
 libsepol/src/module_to_cil.c | 593 +++++++++++++++++++++----------------------
 1 file changed, 283 insertions(+), 310 deletions(-)

Comments

Nicolas Iooss March 28, 2017, 9:22 p.m. UTC | #1
On Tue, Mar 28, 2017 at 7:28 PM, James Carter <jwcart2@tycho.nsa.gov> wrote:
> CIL does not allow type or role sets in certain rules (such as allow
> rules). It does, however, allow sets in typeattributeset and
> roleattributeset statements. Because of this, when module_to_cil
> translates a policy into CIL, it creates a new attribute for each
> set that it encounters. But often the same set is used multiple times
> which means that more attributes are created then necessary. As the
> number of attributes increases the time required for the kernel to
> make each policy decision increases which can be a problem.
>
> To help reduce the number of attributes in a kernel policy,
> when module_to_cil encounters a role or type set search to see if the
> set was encountered already and, if it was, use the previously
> generated attribute instead of creating a new one.
>
> Testing on Android and Refpolicy policies show that this reduces the
> number of attributes generated by about 40%.
>
> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
> ---
>  libsepol/src/module_to_cil.c | 593 +++++++++++++++++++++----------------------
>  1 file changed, 283 insertions(+), 310 deletions(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 6c33b94..4ea8a83 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
>
> [...]
>
> +static char *get_new_attr_name(struct policydb *pdb, int is_type)
>  {
>         static unsigned int num_attrs = 0;
> -       int rc = -1;
>         int len, rlen;
> -       const char *attr_infix;
> -       char *attr;
> +       char *infix;
> +       char *attr_name = NULL;

Why is infix "char *" instead of "const char *", like attr_infix was?
I am seeing a compiler warning with -Wwrite-strings ("error:
assignment discards ‘const’ qualifier from pointer target type" on
"infix = TYPEATTR_INFIX").

Cheers,
Nicolas
James Carter March 29, 2017, 12:38 p.m. UTC | #2
On 03/28/2017 05:22 PM, Nicolas Iooss wrote:
> On Tue, Mar 28, 2017 at 7:28 PM, James Carter <jwcart2@tycho.nsa.gov> wrote:
>> CIL does not allow type or role sets in certain rules (such as allow
>> rules). It does, however, allow sets in typeattributeset and
>> roleattributeset statements. Because of this, when module_to_cil
>> translates a policy into CIL, it creates a new attribute for each
>> set that it encounters. But often the same set is used multiple times
>> which means that more attributes are created then necessary. As the
>> number of attributes increases the time required for the kernel to
>> make each policy decision increases which can be a problem.
>>
>> To help reduce the number of attributes in a kernel policy,
>> when module_to_cil encounters a role or type set search to see if the
>> set was encountered already and, if it was, use the previously
>> generated attribute instead of creating a new one.
>>
>> Testing on Android and Refpolicy policies show that this reduces the
>> number of attributes generated by about 40%.
>>
>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
>> ---
>>  libsepol/src/module_to_cil.c | 593 +++++++++++++++++++++----------------------
>>  1 file changed, 283 insertions(+), 310 deletions(-)
>>
>> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
>> index 6c33b94..4ea8a83 100644
>> --- a/libsepol/src/module_to_cil.c
>> +++ b/libsepol/src/module_to_cil.c
>>
>> [...]
>>
>> +static char *get_new_attr_name(struct policydb *pdb, int is_type)
>>  {
>>         static unsigned int num_attrs = 0;
>> -       int rc = -1;
>>         int len, rlen;
>> -       const char *attr_infix;
>> -       char *attr;
>> +       char *infix;
>> +       char *attr_name = NULL;
>
> Why is infix "char *" instead of "const char *", like attr_infix was?
> I am seeing a compiler warning with -Wwrite-strings ("error:
> assignment discards ‘const’ qualifier from pointer target type" on
> "infix = TYPEATTR_INFIX").
>

It should be "const char *".

Thanks,
Jim

> Cheers,
> Nicolas
>
diff mbox

Patch

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 6c33b94..4ea8a83 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -174,12 +174,9 @@  struct role_list_node {
 };
 
 struct attr_list_node {
-	char *attribute;
+	char *attr_name;
 	int is_type;
-	union {
-		struct type_set *ts;
-		struct role_set *rs;
-	} set;
+	void *set;
 };
 
 struct list_node {
@@ -237,7 +234,7 @@  static void attr_list_destroy(struct list **attr_list)
 	while (curr != NULL) {
 		attr = curr->data;
 		if (attr != NULL) {
-			free(attr->attribute);
+			free(attr->attr_name);
 		}
 
 		free(curr->data);
@@ -717,54 +714,85 @@  static int num_digits(int n)
 	return num;
 }
 
-static int set_to_cil_attr(struct policydb *pdb, int is_type, char ***names, uint32_t *num_names)
+static int ebitmap_to_cil(struct policydb *pdb, struct ebitmap *map, int type)
+{
+	struct ebitmap_node *node;
+	uint32_t i;
+	char **val_to_name = pdb->sym_val_to_name[type];
+
+	ebitmap_for_each_bit(map, node, i) {
+		if (!ebitmap_get_bit(map, i)) {
+			continue;
+		}
+		cil_printf("%s ", val_to_name[i]);
+	}
+
+	return 0;
+}
+
+static char *get_new_attr_name(struct policydb *pdb, int is_type)
 {
 	static unsigned int num_attrs = 0;
-	int rc = -1;
 	int len, rlen;
-	const char *attr_infix;
-	char *attr;
+	char *infix;
+	char *attr_name = NULL;
 
 	num_attrs++;
 
 	if (is_type) {
-		attr_infix = TYPEATTR_INFIX;
+		infix = TYPEATTR_INFIX;
 	} else {
-		attr_infix = ROLEATTR_INFIX;
+		infix = ROLEATTR_INFIX;
 	}
 
-	len = strlen(pdb->name) + strlen(attr_infix) + num_digits(num_attrs) + 1;
-	attr = malloc(len);
-	if (attr == NULL) {
+	len = strlen(pdb->name) + strlen(infix) + num_digits(num_attrs) + 1;
+	attr_name = malloc(len);
+	if (!attr_name) {
 		log_err("Out of memory");
-		rc = -1;
 		goto exit;
 	}
-	rlen = snprintf(attr, len, "%s%s%i", pdb->name, attr_infix, num_attrs);
+
+	rlen = snprintf(attr_name, len, "%s%s%i", pdb->name, infix, num_attrs);
 	if (rlen < 0 || rlen >= len) {
 		log_err("Failed to generate attribute name");
-		rc = -1;
+		free(attr_name);
+		attr_name = NULL;
 		goto exit;
 	}
 
-	*names = malloc(sizeof(**names));
-	if (*names == NULL) {
+exit:
+	return attr_name;
+}
+
+static int cil_add_attr_to_list(struct list *attr_list, char *attr_name, int is_type, void *set)
+{
+	struct attr_list_node *attr_list_node = NULL;
+	int rc = 0;
+
+	attr_list_node = calloc(1, sizeof(*attr_list_node));
+	if (attr_list_node == NULL) {
 		log_err("Out of memory");
 		rc = -1;
 		goto exit;
 	}
 
+	rc = list_prepend(attr_list, attr_list_node);
+	if (rc != 0) {
+		goto exit;
+	}
 
-	*names[0] = attr;
-	*num_names = 1;
+	attr_list_node->attr_name = attr_name;
+	attr_list_node->is_type = is_type;
+	attr_list_node->set = set;
 
-	rc = 0;
+	return rc;
 
 exit:
+	free(attr_list_node);
 	return rc;
 }
 
-static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, struct ebitmap *pos, struct ebitmap *neg, uint32_t flags, char *attr)
+static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, void *set, char *attr_name)
 {
 	// CIL doesn't support anonymous positive/negative/complemented sets.  So
 	// instead we create a CIL type/roleattributeset that matches the set. If
@@ -773,25 +801,40 @@  static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, st
 	// in the negative set. Additonally, if the set is complemented, then wrap
 	// the whole thing with a negation.
 
-	int rc = 0;
 	struct ebitmap_node *node;
-	unsigned int i;
-	const char *statement;
-	int has_positive = pos && (ebitmap_cardinality(pos) > 0);
-	int has_negative = neg && (ebitmap_cardinality(neg) > 0);
+	struct ebitmap *pos, *neg;
+	uint32_t flags;
+	unsigned i;
+	struct type_set *ts;
+	struct role_set *rs;
+	int has_positive, has_negative;
+	const char *kind;
 	char **val_to_name;
+	int rc = 0;
 
 	if (is_type) {
-		statement = "type";
+		kind = "type";
 		val_to_name = pdb->p_type_val_to_name;
+		ts = (struct type_set *)set;
+		pos = &ts->types;
+		neg = &ts->negset;
+		flags = ts->flags;
+		has_positive = pos && (ebitmap_length(pos) > 0);
+		has_negative = neg && (ebitmap_length(neg) > 0);
 	} else {
-		statement = "role";
+		kind = "role";
 		val_to_name = pdb->p_role_val_to_name;
+		rs = (struct role_set *)set;
+		pos = &rs->roles;
+		neg = NULL;
+		flags = rs->flags;
+		has_positive = pos && (ebitmap_length(pos) > 0);
+		has_negative = 0;
 	}
 
-	cil_println(indent, "(%sattribute %s)", statement, attr);
+	cil_println(indent, "(%sattribute %s)", kind, attr_name);
 	cil_indent(indent);
-	cil_printf("(%sattributeset %s ", statement, attr);
+	cil_printf("(%sattributeset %s ", kind, attr_name);
 
 	if (flags & TYPE_STAR) {
 		cil_printf("(all)");
@@ -842,184 +885,150 @@  static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, st
 	return rc;
 }
 
-static int ebitmap_to_cil(struct policydb *pdb, struct ebitmap *map, int type)
+static int cil_print_attr_list(int indent, struct policydb *pdb, struct list *attr_list)
 {
-	struct ebitmap_node *node;
-	uint32_t i;
-	char **val_to_name = pdb->sym_val_to_name[type];
+	struct list_node *curr;
+	struct attr_list_node *node;
+	int rc = 0;
 
-	ebitmap_for_each_bit(map, node, i) {
-		if (!ebitmap_get_bit(map, i)) {
-			continue;
+	for (curr = attr_list->head; curr != NULL; curr = curr->next) {
+		node = curr->data;
+		rc = cil_print_attr_strs(indent, pdb, node->is_type, node->set, node->attr_name);
+		if (rc != 0) {
+			return rc;
 		}
-		cil_printf("%s ", val_to_name[i]);
 	}
 
-	return 0;
+	return rc;
 }
 
-static int ebitmap_to_names(char** vals_to_names, struct ebitmap map, char ***names, uint32_t *num_names)
+static char *search_attr_list(struct list *attr_list, int is_type, void *set)
 {
-	int rc = -1;
-	struct ebitmap_node *node;
-	uint32_t i;
-	uint32_t num = 0;
-	uint32_t max = 8;
-	char **name_arr = NULL;
+	struct list_node *curr;
+	struct attr_list_node *node;
+	struct role_set *rs1, *rs2;
+	struct type_set *ts1, *ts2;
 
-	name_arr = malloc(sizeof(*name_arr) * max);
-	if (name_arr == NULL) {
-		log_err("Out of memory");
-		rc = -1;
-		goto exit;
+	if (is_type) {
+		ts1 = (struct type_set *)set;
+	} else {
+		rs1 = (struct role_set *)set;
 	}
 
-	ebitmap_for_each_bit(&map, node, i) {
-		if (!ebitmap_get_bit(&map, i)) {
+	for (curr = attr_list->head; curr != NULL; curr = curr->next) {
+		node = curr->data;
+		if (node->is_type != is_type)
 			continue;
+		if (is_type) {
+			ts2 = (struct type_set *)node->set;
+			if (ts1->flags != ts2->flags)
+				continue;
+			if (ebitmap_cmp(&ts1->negset, &ts2->negset) == 0)
+				continue;
+			if (ebitmap_cmp(&ts1->types, &ts2->types) == 0)
+				continue;
+			return node->attr_name;
+		} else {
+			rs2 = (struct role_set *)node->set;
+			if (rs1->flags != rs2->flags)
+				continue;
+			if (ebitmap_cmp(&rs1->roles, &rs2->roles) == 0)
+				continue;
+			return node->attr_name;
 		}
-
-		if (num + 1 == max) {
-			max *= 2;
-			name_arr = realloc(name_arr, sizeof(*name_arr) * max);
-			if (name_arr == NULL) {
-				log_err("Out of memory");
-				rc = -1;
-				goto exit;
-			}
-		}
-
-		name_arr[num] = strdup(vals_to_names[i]);
-		if (name_arr[num] == NULL) {
-			log_err("Out of memory");
-			rc = -1;
-			goto exit;
-		}
-		num++;
 	}
 
-	*names = name_arr;
-	*num_names = num;
-
-	return 0;
-
-exit:
-	for (i = 0; i < num; i++) {
-		free(name_arr[i]);
-	}
-	free(name_arr);
-	return rc;
+	return NULL;
 }
 
-static int cil_add_attr_to_list(struct list *attr_list, char *attribute, int is_type, void *set)
+static int set_to_names(struct policydb *pdb, int is_type, void *set, struct list *attr_list, char ***names, int *num_names)
 {
-	struct attr_list_node *attr_list_node = NULL;
-	int rc = -1;
+	char *attr_name = NULL;
+	int rc = 0;
 
-	attr_list_node = calloc(1, sizeof(*attr_list_node));
-	if (attr_list_node == NULL) {
-		log_err("Out of memory");
-		rc = -1;
-		goto exit;
-	}
+	*names = NULL;
+	*num_names = 0;
 
-	rc = list_prepend(attr_list, attr_list_node);
-	if (rc != 0) {
-		goto exit;
+	attr_name = search_attr_list(attr_list, is_type, set);
+
+	if (!attr_name) {
+		attr_name = get_new_attr_name(pdb, is_type);
+		if (!attr_name) {
+			rc = -1;
+			goto exit;
+		}
+
+		rc = cil_add_attr_to_list(attr_list, attr_name, is_type, set);
+		if (rc != 0) {
+			free(attr_name);
+			goto exit;
+		}
 	}
 
-	attr_list_node->attribute = strdup(attribute);
-	if (attr_list_node->attribute == NULL) {
+	*names = malloc(sizeof(char *));
+	if (!*names) {
 		log_err("Out of memory");
+		free(attr_name);
 		rc = -1;
 		goto exit;
 	}
-
-	attr_list_node->is_type = is_type;
-	if (is_type) {
-		attr_list_node->set.ts = set;
-	} else {
-		attr_list_node->set.rs = set;
-	}
-
-	return rc;
+	*names[0] = attr_name;
+	*num_names = 1;
 
 exit:
-	if (attr_list_node != NULL) {
-		free(attr_list_node->attribute);
-	}
-	free(attr_list_node);
 	return rc;
 }
 
-/* generated_attribute is only set if a new attribute was generated in set_to_cil_attr */
-static int typeset_to_names(struct policydb *pdb, struct type_set *ts, char ***names, uint32_t *num_names, char **generated_attribute)
+static int ebitmap_to_names(struct ebitmap *map, char **vals_to_names, char ***names, int *num_names)
 {
-	int rc = -1;
-	if (ebitmap_cardinality(&ts->negset) > 0 || ts->flags != 0) {
-		rc = set_to_cil_attr(pdb, 1, names, num_names);
-		if (rc != 0) {
-			goto exit;
-		}
+	int rc = 0;
+	struct ebitmap_node *node;
+	uint32_t i;
+	uint32_t num;
+	char **name_arr;
 
-		*generated_attribute = *names[0];
-	} else {
-		rc = ebitmap_to_names(pdb->p_type_val_to_name, ts->types, names, num_names);
-		if (rc != 0) {
-			goto exit;
-		}
+	num = 0;
+	ebitmap_for_each_bit(map, node, i) {
+		if (ebitmap_get_bit(map, i))
+			num++;
 	}
 
-	return 0;
-exit:
-	return rc;
-}
-
-/* generated_attribute is only set if a new attribute was generated in set_to_cil_attr */
-static int roleset_to_names(struct policydb *pdb, struct role_set *rs, char ***names, uint32_t *num_names, char **generated_attribute)
-{
-	int rc = -1;
-	if (rs->flags != 0) {
-		rc = set_to_cil_attr(pdb, 0, names, num_names);
-		if (rc != 0) {
-			goto exit;
-		}
+	name_arr = malloc(sizeof(*name_arr) * num);
+	if (name_arr == NULL) {
+		log_err("Out of memory");
+		rc = -1;
+		goto exit;
+	}
 
-		*generated_attribute = *names[0];
-	} else {
-		rc = ebitmap_to_names(pdb->p_role_val_to_name, rs->roles, names, num_names);
-		if (rc != 0) {
-			goto exit;
+	num = 0;
+	ebitmap_for_each_bit(map, node, i) {
+		if (ebitmap_get_bit(map, i)) {
+			name_arr[num] = vals_to_names[i];
+			num++;
 		}
 	}
 
-	return 0;
+	*names = name_arr;
+	*num_names = num;
+
 exit:
 	return rc;
 }
 
-static int process_roleset(int indent, struct policydb *pdb, struct role_set *rs, struct list *attr_list, char ***type_names, uint32_t *num_type_names)
+static int process_roleset(struct policydb *pdb, struct role_set *rs, struct list *attr_list, char ***names, int *num_names)
 {
-	int rc = -1;
-	char *generated_attribute = NULL;
-	*num_type_names = 0;
-
-	rc = roleset_to_names(pdb, rs, type_names, num_type_names, &generated_attribute);
-	if (rc != 0) {
-		goto exit;
-	}
+	int rc = 0;
 
-	if (generated_attribute == NULL) {
-		goto exit;
-	}
+	*names = NULL;
+	*num_names = 0;
 
-	if (attr_list == NULL) {
-		rc = cil_print_attr_strs(indent, pdb, 0, &rs->roles, NULL, rs->flags, generated_attribute);
+	if (rs->flags) {
+		rc = set_to_names(pdb, 0, &rs->roles, attr_list, names, num_names);
 		if (rc != 0) {
 			goto exit;
 		}
 	} else {
-		rc = cil_add_attr_to_list(attr_list, generated_attribute, 0, rs);
+		rc = ebitmap_to_names(&rs->roles, pdb->p_role_val_to_name, names, num_names);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1029,29 +1038,20 @@  exit:
 	return rc;
 }
 
-static int process_typeset(int indent, struct policydb *pdb, struct type_set *ts, struct list *attr_list, char ***type_names, uint32_t *num_type_names)
+static int process_typeset(struct policydb *pdb, struct type_set *ts, struct list *attr_list, char ***names, int *num_names)
 {
-	int rc = -1;
-	char *generated_attribute = NULL;
-	*num_type_names = 0;
-
-	rc = typeset_to_names(pdb, ts, type_names, num_type_names, &generated_attribute);
-	if (rc != 0) {
-		goto exit;
-	}
+	int rc = 0;
 
-	if (generated_attribute == NULL) {
-		rc = 0;
-		goto exit;
-	}
+	*names = NULL;
+	*num_names = 0;
 
-	if (attr_list == NULL) {
-		rc = cil_print_attr_strs(indent, pdb, 1, &ts->types, &ts->negset, ts->flags, generated_attribute);
+	if (ebitmap_length(&ts->negset) > 0 || ts->flags != 0) {
+		rc = set_to_names(pdb, 1, ts, attr_list, names, num_names);
 		if (rc != 0) {
 			goto exit;
 		}
 	} else {
-		rc = cil_add_attr_to_list(attr_list, generated_attribute, 1, ts);
+		rc = ebitmap_to_names(&ts->types, pdb->p_type_val_to_name, names, num_names);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1061,18 +1061,9 @@  exit:
 	return rc;
 }
 
-static void names_destroy(char ***names, uint32_t *num_names)
+static void names_destroy(char ***names, int *num_names)
 {
-	char **arr = *names;
-	uint32_t num = *num_names;
-	uint32_t i;
-
-	for (i = 0; i < num; i++) {
-		free(arr[i]);
-		arr[i] = NULL;
-	}
-	free(arr);
-
+	free(*names);
 	*names = NULL;
 	*num_names = 0;
 }
@@ -1081,10 +1072,16 @@  static int roletype_role_in_ancestor_to_cil(struct policydb *pdb, struct stack *
 {
 	struct list_node *curr;
 	char **tnames = NULL;
-	uint32_t num_tnames, i;
+	int num_tnames, i;
 	struct role_list_node *role_node = NULL;
 	int rc;
 	struct type_set *ts;
+	struct list *attr_list = NULL;
+
+	rc = list_init(&attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
 
 	curr = role_list->head;
 	for (curr = role_list->head; curr != NULL; curr = curr->next) {
@@ -1094,7 +1091,7 @@  static int roletype_role_in_ancestor_to_cil(struct policydb *pdb, struct stack *
 		}
 
 		ts = &role_node->role->types;
-		rc = process_typeset(indent, pdb, ts, NULL, &tnames, &num_tnames);
+		rc = process_typeset(pdb, ts, attr_list, &tnames, &num_tnames);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1106,9 +1103,13 @@  static int roletype_role_in_ancestor_to_cil(struct policydb *pdb, struct stack *
 		names_destroy(&tnames, &num_tnames);
 	}
 
-	rc = 0;
+	rc = cil_print_attr_list(indent, pdb, attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
 
 exit:
+	attr_list_destroy(&attr_list);
 	return rc;
 }
 
@@ -1168,10 +1169,7 @@  static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a
 	struct avrule *avrule;
 	char **snames = NULL;
 	char **tnames = NULL;
-	uint32_t num_snames;
-	uint32_t num_tnames;
-	uint32_t s;
-	uint32_t t;
+	int s, t, num_snames, num_tnames;
 	struct type_set *ts;
 
 	for (avrule = avrule_list; avrule != NULL; avrule = avrule->next) {
@@ -1181,13 +1179,13 @@  static int avrule_list_to_cil(int indent, struct policydb *pdb, struct avrule *a
 		}
 
 		ts = &avrule->stypes;
-		rc = process_typeset(indent, pdb, ts, attr_list, &snames, &num_snames);
+		rc = process_typeset(pdb, ts, attr_list, &snames, &num_snames);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		ts = &avrule->ttypes;
-		rc = process_typeset(indent, pdb, ts, attr_list, &tnames, &num_tnames);
+		rc = process_typeset(pdb, ts, attr_list, &tnames, &num_tnames);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1236,7 +1234,7 @@  exit:
 
 static int cond_expr_to_cil(int indent, struct policydb *pdb, struct cond_expr *cond_expr, uint32_t flags)
 {
-	int rc = -1;
+	int rc = 0;
 	struct cond_expr *curr;
 	struct stack *stack = NULL;
 	int len = 0;
@@ -1377,50 +1375,10 @@  exit:
 	return rc;
 }
 
-static int cil_print_attr_list(int indent, struct policydb *pdb, struct list *attr_list)
+static int cond_list_to_cil(int indent, struct policydb *pdb, struct cond_node *cond_list, struct list *attr_list)
 {
-	struct list_node *curr;
-	struct attr_list_node *attr_list_node;
 	int rc = 0;
-	struct type_set *ts;
-	struct role_set *rs;
-	char *generated_attribute;
-
-	for (curr = attr_list->head; curr != NULL; curr = curr->next) {
-		attr_list_node = curr->data;
-		generated_attribute = attr_list_node->attribute;
-		if (generated_attribute == NULL) {
-			return -1;
-		}
-
-		if (attr_list_node->is_type) {
-			ts = attr_list_node->set.ts;
-			rc = cil_print_attr_strs(indent, pdb, 1, &ts->types, &ts->negset, ts->flags, generated_attribute);
-			if (rc != 0) {
-				return rc;
-			}
-		} else {
-			rs = attr_list_node->set.rs;
-			rc = cil_print_attr_strs(indent, pdb, 0, &rs->roles, NULL, rs->flags, generated_attribute);
-			if (rc != 0) {
-				return rc;
-			}
-		}
-	}
-
-	return rc;
-}
-
-static int cond_list_to_cil(int indent, struct policydb *pdb, struct cond_node *cond_list)
-{
-	int rc = -1;
 	struct cond_node *cond;
-	struct list *attr_list = NULL;
-
-	rc = list_init(&attr_list);
-	if (rc != 0) {
-		goto exit;
-	}
 
 	for (cond = cond_list; cond != NULL; cond = cond->next) {
 
@@ -1450,38 +1408,34 @@  static int cond_list_to_cil(int indent, struct policydb *pdb, struct cond_node *
 		cil_println(indent, ")");
 	}
 
-	rc = cil_print_attr_list(indent, pdb, attr_list);
-
 exit:
-	attr_list_destroy(&attr_list);
 	return rc;
 }
 
-static int role_trans_to_cil(int indent, struct policydb *pdb, struct role_trans_rule *rules)
+static int role_trans_to_cil(int indent, struct policydb *pdb, struct role_trans_rule *rules, struct list *role_attr_list, struct list *type_attr_list)
 {
-	int rc = -1;
+	int rc = 0;
 	struct role_trans_rule *rule;
 	char **role_names = NULL;
-	uint32_t num_role_names = 0;
+	int num_role_names = 0;
+	int role;
 	char **type_names = NULL;
-	uint32_t num_type_names = 0;
-	uint32_t type;
-	uint32_t role;
+	int num_type_names = 0;
+	int type;
 	uint32_t i;
 	struct ebitmap_node *node;
 	struct type_set *ts;
 	struct role_set *rs;
 
-
 	for (rule = rules; rule != NULL; rule = rule->next) {
 		rs = &rule->roles;
-		rc = process_roleset(indent, pdb, rs, NULL, &role_names, &num_role_names);
+		rc = process_roleset(pdb, rs, role_attr_list, &role_names, &num_role_names);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		ts = &rule->types;
-		rc = process_typeset(indent, pdb, ts, NULL, &type_names, &num_type_names);
+		rc = process_typeset(pdb, ts, type_attr_list, &type_names, &num_type_names);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1492,10 +1446,10 @@  static int role_trans_to_cil(int indent, struct policydb *pdb, struct role_trans
 					if (!ebitmap_get_bit(&rule->classes, i)) {
 						continue;
 					}
-					cil_println(indent, "(roletransition %s %s %s %s)", role_names[role],
-					                                                    type_names[type],
-					                                                    pdb->p_class_val_to_name[i],
-					                                                    pdb->p_role_val_to_name[rule->new_role - 1]);
+					cil_println(indent, "(roletransition %s %s %s %s)",
+						    role_names[role], type_names[type],
+						    pdb->p_class_val_to_name[i],
+						    pdb->p_role_val_to_name[rule->new_role - 1]);
 				}
 			}
 		}
@@ -1504,8 +1458,6 @@  static int role_trans_to_cil(int indent, struct policydb *pdb, struct role_trans
 		names_destroy(&type_names, &num_type_names);
 	}
 
-	rc = 0;
-
 exit:
 	names_destroy(&role_names, &num_role_names);
 	names_destroy(&type_names, &num_type_names);
@@ -1513,27 +1465,26 @@  exit:
 	return rc;
 }
 
-static int role_allows_to_cil(int indent, struct policydb *pdb, struct role_allow_rule *rules)
+static int role_allows_to_cil(int indent, struct policydb *pdb, struct role_allow_rule *rules, struct list *attr_list)
 {
 	int rc = -1;
 	struct role_allow_rule *rule;
 	char **roles = NULL;
-	uint32_t num_roles = 0;
+	int num_roles = 0;
 	char **new_roles = NULL;
-	uint32_t num_new_roles = 0;
-	uint32_t i;
-	uint32_t j;
+	int num_new_roles = 0;
+	int i,j;
 	struct role_set *rs;
 
 	for (rule = rules; rule != NULL; rule = rule->next) {
 		rs = &rule->roles;
-		rc = process_roleset(indent, pdb, rs, NULL, &roles, &num_roles);
+		rc = process_roleset(pdb, rs, attr_list, &roles, &num_roles);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		rs = &rule->new_roles;
-		rc = process_roleset(indent, pdb, rs, NULL, &new_roles, &num_new_roles);
+		rc = process_roleset(pdb, rs, attr_list, &new_roles, &num_new_roles);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1557,18 +1508,18 @@  exit:
 	return rc;
 }
 
-static int range_trans_to_cil(int indent, struct policydb *pdb, struct range_trans_rule *rules)
+static int range_trans_to_cil(int indent, struct policydb *pdb, struct range_trans_rule *rules, struct list *attr_list)
 {
 	int rc = -1;
 	struct range_trans_rule *rule;
 	char **stypes = NULL;
-	uint32_t num_stypes = 0;
+	int num_stypes = 0;
+	int stype;
 	char **ttypes = NULL;
-	uint32_t num_ttypes = 0;
+	int num_ttypes = 0;
+	int ttype;
 	struct ebitmap_node *node;
 	uint32_t i;
-	uint32_t stype;
-	uint32_t ttype;
 	struct type_set *ts;
 
 	if (!pdb->mls) {
@@ -1577,13 +1528,13 @@  static int range_trans_to_cil(int indent, struct policydb *pdb, struct range_tra
 
 	for (rule = rules; rule != NULL; rule = rule->next) {
 		ts = &rule->stypes;
-		rc = process_typeset(indent, pdb, ts, NULL, &stypes, &num_stypes);
+		rc = process_typeset(pdb, ts, attr_list, &stypes, &num_stypes);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		ts = &rule->ttypes;
-		rc = process_typeset(indent, pdb, ts, NULL, &ttypes, &num_ttypes);
+		rc = process_typeset(pdb, ts, attr_list, &ttypes, &num_ttypes);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -1631,39 +1582,38 @@  exit:
 	return rc;
 }
 
-static int filename_trans_to_cil(int indent, struct policydb *pdb, struct filename_trans_rule *rules)
+static int filename_trans_to_cil(int indent, struct policydb *pdb, struct filename_trans_rule *rules, struct list *attr_list)
 {
 	int rc = -1;
 	char **stypes = NULL;
-	uint32_t num_stypes = 0;
+	int num_stypes = 0;
+	int stype;
 	char **ttypes = NULL;
-	uint32_t num_ttypes = 0;
-	uint32_t stype;
-	uint32_t ttype;
+	int num_ttypes = 0;
+	int ttype;
 	struct type_set *ts;
-
 	struct filename_trans_rule *rule;
 
 	for (rule = rules; rule != NULL; rule = rule->next) {
 		ts = &rule->stypes;
-		rc = process_typeset(indent, pdb, ts, NULL, &stypes, &num_stypes);
+		rc = process_typeset(pdb, ts, attr_list, &stypes, &num_stypes);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		ts = &rule->ttypes;
-		rc = process_typeset(indent, pdb, ts, NULL, &ttypes, &num_ttypes);
+		rc = process_typeset(pdb, ts, attr_list, &ttypes, &num_ttypes);
 		if (rc != 0) {
 			goto exit;
 		}
 
 		for (stype = 0; stype < num_stypes; stype++) {
 			for (ttype = 0; ttype < num_ttypes; ttype++) {
-				cil_println(indent, "(typetransition %s %s %s \"%s\" %s)", stypes[stype],
-				                                                       ttypes[ttype],
-				                                                       pdb->p_class_val_to_name[rule->tclass - 1],
-				                                                       rule->name,
-				                                                       pdb->p_type_val_to_name[rule->otype - 1]);
+				cil_println(indent, "(typetransition %s %s %s \"%s\" %s)",
+					    stypes[stype], ttypes[ttype],
+					    pdb->p_class_val_to_name[rule->tclass - 1],
+					    rule->name,
+					    pdb->p_type_val_to_name[rule->otype - 1]);
 			}
 		}
 
@@ -1738,7 +1688,7 @@  exit:
 }
 
 
-static int constraint_expr_to_string(int indent, struct policydb *pdb, struct constraint_expr *exprs, char **expr_string)
+static int constraint_expr_to_string(struct policydb *pdb, struct constraint_expr *exprs, char **expr_string)
 {
 	int rc = -1;
 	struct constraint_expr *expr;
@@ -1755,7 +1705,7 @@  static int constraint_expr_to_string(int indent, struct policydb *pdb, struct co
 	const char *attr2;
 	char *names;
 	char **name_list = NULL;
-	uint32_t num_names = 0;
+	int num_names = 0;
 	struct type_set *ts;
 
 	rc = stack_init(&stack);
@@ -1817,17 +1767,17 @@  static int constraint_expr_to_string(int indent, struct policydb *pdb, struct co
 			} else {
 				if (expr->attr & CEXPR_TYPE) {
 					ts = expr->type_names;
-					rc = process_typeset(indent, pdb, ts, NULL, &name_list, &num_names);
+					rc = ebitmap_to_names(&ts->types, pdb->p_type_val_to_name, &name_list, &num_names);
 					if (rc != 0) {
 						goto exit;
 					}
 				} else if (expr->attr & CEXPR_USER) {
-					rc = ebitmap_to_names(pdb->p_user_val_to_name, expr->names, &name_list, &num_names);
+					rc = ebitmap_to_names(&expr->names, pdb->p_user_val_to_name, &name_list, &num_names);
 					if (rc != 0) {
 						goto exit;
 					}
 				} else if (expr->attr & CEXPR_ROLE) {
-					rc = ebitmap_to_names(pdb->p_role_val_to_name, expr->names, &name_list, &num_names);
+					rc = ebitmap_to_names(&expr->names, pdb->p_role_val_to_name, &name_list, &num_names);
 					if (rc != 0) {
 						goto exit;
 					}
@@ -1968,7 +1918,7 @@  static int constraints_to_cil(int indent, struct policydb *pdb, char *classkey,
 
 	for (node = constraints; node != NULL; node = node->next) {
 
-		rc = constraint_expr_to_string(indent, pdb, node->expr, &expr);
+		rc = constraint_expr_to_string(pdb, node->expr, &expr);
 		if (rc != 0) {
 			goto exit;
 		}
@@ -2126,10 +2076,17 @@  static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
 	int rc = -1;
 	struct ebitmap_node *node;
 	uint32_t i;
+	int j;
 	char **types = NULL;
-	uint32_t num_types = 0;
+	int num_types = 0;
 	struct role_datum *role = datum;
 	struct type_set *ts;
+	struct list *attr_list = NULL;
+
+	rc = list_init(&attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
 
 	if (scope == SCOPE_REQ) {
 		// if a role/roleattr is in the REQ scope, then it could cause an
@@ -2183,14 +2140,14 @@  static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
 		}
 
 		ts = &role->types;
-		rc = process_typeset(indent, pdb, ts, NULL, &types, &num_types);
+		rc = process_typeset(pdb, ts, attr_list, &types, &num_types);
 		if (rc != 0) {
 			goto exit;
 		}
 
-		for (i = 0; i < num_types; i++) {
-			if (is_id_in_scope(pdb, decl_stack, types[i], SYM_TYPES)) {
-				cil_println(indent, "(roletype %s %s)", key, types[i]);
+		for (j = 0; j < num_types; j++) {
+			if (is_id_in_scope(pdb, decl_stack, types[j], SYM_TYPES)) {
+				cil_println(indent, "(roletype %s %s)", key, types[j]);
 			}
 		}
 
@@ -2217,15 +2174,15 @@  static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
 		}
 
 		ts = &role->types;
-		rc = process_typeset(indent, pdb, ts, NULL, &types, &num_types);
+		rc = process_typeset(pdb, ts, attr_list, &types, &num_types);
 		if (rc != 0) {
 			goto exit;
 		}
 
 
-		for (i = 0; i < num_types; i++) {
-			if (is_id_in_scope(pdb, decl_stack, types[i], SYM_TYPES)) {
-				cil_println(indent, "(roletype %s %s)", key, types[i]);
+		for (j = 0; j < num_types; j++) {
+			if (is_id_in_scope(pdb, decl_stack, types[j], SYM_TYPES)) {
+				cil_println(indent, "(roletype %s %s)", key, types[j]);
 			}
 		}
 
@@ -2237,8 +2194,13 @@  static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
 		goto exit;
 	}
 
-	rc = 0;
+	rc = cil_print_attr_list(indent, pdb, attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
+
 exit:
+	attr_list_destroy(&attr_list);
 	names_destroy(&types, &num_types);
 
 	return rc;
@@ -3601,11 +3563,16 @@  static int block_to_cil(struct policydb *pdb, struct avrule_block *block, struct
 {
 	int rc = -1;
 	struct avrule_decl *decl;
-	struct list *attr_list = NULL;
+	struct list *type_attr_list = NULL;
+	struct list *role_attr_list = NULL;
 
 	decl = block->branch_list;
 
-	rc = list_init(&attr_list);
+	rc = list_init(&type_attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
+	rc = list_init(&role_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
@@ -3630,43 +3597,49 @@  static int block_to_cil(struct policydb *pdb, struct avrule_block *block, struct
 		goto exit;
 	}
 
-	rc = avrule_list_to_cil(indent, pdb, decl->avrules, attr_list);
+	rc = avrule_list_to_cil(indent, pdb, decl->avrules, type_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = role_trans_to_cil(indent, pdb, decl->role_tr_rules);
+	rc = role_trans_to_cil(indent, pdb, decl->role_tr_rules, role_attr_list, type_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = role_allows_to_cil(indent, pdb, decl->role_allow_rules);
+	rc = role_allows_to_cil(indent, pdb, decl->role_allow_rules, role_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = range_trans_to_cil(indent, pdb, decl->range_tr_rules);
+	rc = range_trans_to_cil(indent, pdb, decl->range_tr_rules, type_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = filename_trans_to_cil(indent, pdb, decl->filename_trans_rules);
+	rc = filename_trans_to_cil(indent, pdb, decl->filename_trans_rules, type_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = cond_list_to_cil(indent, pdb, decl->cond_list);
+	rc = cond_list_to_cil(indent, pdb, decl->cond_list, type_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
-	rc = cil_print_attr_list(indent, pdb, attr_list);
+	rc = cil_print_attr_list(indent, pdb, type_attr_list);
+	if (rc != 0) {
+		goto exit;
+	}
+	rc = cil_print_attr_list(indent, pdb, role_attr_list);
 	if (rc != 0) {
 		goto exit;
 	}
 
 exit:
-	attr_list_destroy(&attr_list);
+	attr_list_destroy(&type_attr_list);
+	attr_list_destroy(&role_attr_list);
+
 	return rc;
 }