Re: [PATCH] Improve SELinux performance when AVC misses.

[KaiGai Kohei <kaigai@ak.jp.nec.com>]

KaiGai Kohei wrote:
>>> I think we have to conform to it, and if it makes it harder to read,
>>> possibly that suggests that we need to rework the code in some way.
>> I'm preparing another version of patch which does not contain any
>> "line over 80 characters" warning. Please wait for a while.
> 
> The following part is the patch which does not contain any "line
> over 80 characters" warnings.

The following part is a result of diff between the previous patch
which contains 80-char warnings and the latest one without them.

I think it is better than I expected.
However, there are several ugly manner like as:

-+				c_iter->startbit = e_startbit & ~(NETLBL_CATMAP_SIZE - 1);
++				c_iter->startbit
++				  = e_startbit & ~(NETLBL_CATMAP_SIZE - 1);
If both side of the formula is in a same line, the width of the line get overs
the 80-char limitation.


++	char *nm;
           :
++				if (head != prev) {
++					nm = policydb.p_cat_val_to_name[prev];
++					len += strlen(nm) + 1;
++				}

If the temporary char * variable is named as "cat_name", the above longest
line overs 80-char limitation, so I named it as "nm".
This naming may be unclear for its purpose.

Thanks,

[kaigai@saba ~]$ diff -NU3 ebitmap-bitops.patch ebitmap-bitops.less-than-80char.patch
 diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
-index ce492a6..3a60186 100644
+index ce492a6..5643bdd 100644
 --- a/security/selinux/ss/ebitmap.c
 +++ b/security/selinux/ss/ebitmap.c
 @@ -10,6 +10,10 @@
@@ -53,7 +59,7 @@

  	if (e_iter == NULL) {
  		*catmap = NULL;
-@@ -104,19 +110,21 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
+@@ -104,19 +110,27 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
  	c_iter->startbit = e_iter->startbit & ~(NETLBL_CATMAP_SIZE - 1);

  	while (e_iter != NULL) {
@@ -66,18 +72,24 @@
 -			c_iter->startbit = e_iter->startbit &
 -				           ~(NETLBL_CATMAP_SIZE - 1);
 +		for (i = 0; i < EBITMAP_UNIT_NUMS; i++) {
-+			unsigned int e_startbit = e_iter->startbit + i * EBITMAP_UNIT_SIZE;
++			unsigned int delta, e_startbit, c_endbit;
 +
-+			if (e_startbit >= c_iter->startbit + NETLBL_CATMAP_SIZE) {
-+				c_iter->next = netlbl_secattr_catmap_alloc(GFP_ATOMIC);
++			e_startbit = e_iter->startbit + i * EBITMAP_UNIT_SIZE;
++			c_endbit = c_iter->startbit + NETLBL_CATMAP_SIZE;
++			if (e_startbit >= c_endbit) {
++				c_iter->next
++				  = netlbl_secattr_catmap_alloc(GFP_ATOMIC);
 +				if (c_iter->next == NULL)
 +					goto netlbl_export_failure;
 +				c_iter = c_iter->next;
-+				c_iter->startbit = e_startbit & ~(NETLBL_CATMAP_SIZE - 1);
++				c_iter->startbit
++				  = e_startbit & ~(NETLBL_CATMAP_SIZE - 1);
 +			}
-+			cmap_idx = (e_startbit - c_iter->startbit) / NETLBL_CATMAP_MAPSIZE;
-+			cmap_sft = (e_startbit - c_iter->startbit) % NETLBL_CATMAP_MAPSIZE;
-+			c_iter->bitmap[cmap_idx] |= e_iter->maps[cmap_idx] << cmap_sft;
++			delta = e_startbit - c_iter->startbit;
++			cmap_idx = delta / NETLBL_CATMAP_MAPSIZE;
++			cmap_sft = delta % NETLBL_CATMAP_MAPSIZE;
++			c_iter->bitmap[cmap_idx]
++				|= e_iter->maps[cmap_idx] << cmap_sft;
 +			e_iter = e_iter->next;
  		}
 -		cmap_idx = (e_iter->startbit - c_iter->startbit) /
@@ -87,7 +99,7 @@
  	}

  	return 0;
-@@ -128,7 +136,7 @@ netlbl_export_failure:
+@@ -128,7 +142,7 @@ netlbl_export_failure:

  /**
   * ebitmap_netlbl_import - Import a NetLabel category bitmap into an ebitmap
@@ -96,7 +108,7 @@
   * @catmap: the NetLabel category bitmap
   *
   * Description:
-@@ -142,36 +150,45 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
+@@ -142,36 +156,50 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
  	struct ebitmap_node *e_iter = NULL;
  	struct ebitmap_node *emap_prev = NULL;
  	struct netlbl_lsm_secattr_catmap *c_iter = catmap;
@@ -118,6 +130,7 @@
  	do {
  		for (c_idx = 0; c_idx < NETLBL_CATMAP_MAPCNT; c_idx++) {
 -			if (c_iter->bitmap[c_idx] == 0)
++			unsigned int delta;
 +			u64 map = c_iter->bitmap[c_idx];
 +
 +			if (!map)
@@ -135,20 +148,24 @@
 -			e_iter->startbit = c_iter->startbit +
 -				           NETLBL_CATMAP_MAPSIZE * c_idx;
 -			e_iter->map = c_iter->bitmap[c_idx];
-+			c_pos = c_iter->startbit + c_idx * NETLBL_CATMAP_MAPSIZE;
-+			if (!e_iter || c_pos >= e_iter->startbit + EBITMAP_SIZE) {
++			c_pos = c_iter->startbit
++				+ c_idx * NETLBL_CATMAP_MAPSIZE;
++			if (!e_iter
++			    || c_pos >= e_iter->startbit + EBITMAP_SIZE) {
 +				e_iter = kzalloc(sizeof(*e_iter), GFP_ATOMIC);
 +				if (!e_iter)
 +					goto netlbl_import_failure;
-+				e_iter->startbit = c_pos - (c_pos % EBITMAP_SIZE);
++				e_iter->startbit
++					= c_pos - (c_pos % EBITMAP_SIZE);
 +				if (emap_prev == NULL)
 +					ebmap->node = e_iter;
 +				else
 +					emap_prev->next = e_iter;
 +				emap_prev = e_iter;
 +			}
-+			e_idx = (c_pos - e_iter->startbit) / EBITMAP_UNIT_SIZE;
-+			e_sft = (c_pos - e_iter->startbit) % EBITMAP_UNIT_SIZE;
++			delta = c_pos - e_iter->startbit;
++			e_idx = delta / EBITMAP_UNIT_SIZE;
++			e_sft = delta % EBITMAP_UNIT_SIZE;
 +			while (map) {
 +				e_iter->maps[e_idx++] |= map & (-1UL);
 +				map >>= EBITMAP_UNIT_SIZE;
@@ -162,7 +179,7 @@
  	else
  		ebitmap_destroy(ebmap);

-@@ -186,6 +203,7 @@ netlbl_import_failure:
+@@ -186,6 +214,7 @@ netlbl_import_failure:
  int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
  {
  	struct ebitmap_node *n1, *n2;
@@ -170,7 +187,7 @@

  	if (e1->highbit < e2->highbit)
  		return 0;
-@@ -197,8 +215,10 @@ int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
+@@ -197,8 +226,10 @@ int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
  			n1 = n1->next;
  			continue;
  		}
@@ -183,7 +200,7 @@

  		n1 = n1->next;
  		n2 = n2->next;
-@@ -219,12 +239,8 @@ int ebitmap_get_bit(struct ebitmap *e, unsigned long bit)
+@@ -219,12 +250,8 @@ int ebitmap_get_bit(struct ebitmap *e, unsigned long bit)

  	n = e->node;
  	while (n && (n->startbit <= bit)) {
@@ -198,7 +215,7 @@
  		n = n->next;
  	}

-@@ -238,31 +254,31 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
+@@ -238,31 +265,35 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
  	prev = NULL;
  	n = e->node;
  	while (n && n->startbit <= bit) {
@@ -222,9 +239,12 @@
 -						else
 -							e->highbit = 0;
 -					}
++				unsigned int s;
++
 +				ebitmap_node_clr_bit(n, bit);
 +
-+				if (find_first_bit(n->maps, EBITMAP_SIZE) < EBITMAP_SIZE)
++				s = find_first_bit(n->maps, EBITMAP_SIZE);
++				if (s < EBITMAP_SIZE)
 +					return 0;
 +
 +				/* drop this node from the bitmap */
@@ -235,7 +255,8 @@
 +					 */
  					if (prev)
 -						prev->next = n->next;
-+						e->highbit = prev->startbit + EBITMAP_SIZE;
++						e->highbit = prev->startbit
++							     + EBITMAP_SIZE;
  					else
 -						e->node = n->next;
 -
@@ -250,7 +271,7 @@
  			}
  			return 0;
  		}
-@@ -277,12 +293,12 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
+@@ -277,12 +308,12 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
  	if (!new)
  		return -ENOMEM;

@@ -266,7 +287,7 @@

  	if (prev) {
  		new->next = prev->next;
-@@ -316,11 +332,11 @@ void ebitmap_destroy(struct ebitmap *e)
+@@ -316,11 +347,11 @@ void ebitmap_destroy(struct ebitmap *e)

  int ebitmap_read(struct ebitmap *e, void *fp)
  {
@@ -282,7 +303,7 @@

  	ebitmap_init(e);

-@@ -328,85 +344,86 @@ int ebitmap_read(struct ebitmap *e, void *fp)
+@@ -328,85 +359,89 @@ int ebitmap_read(struct ebitmap *e, void *fp)
  	if (rc < 0)
  		goto out;

@@ -354,13 +375,16 @@
 +		}
 +
 +		if (!n || startbit >= n->startbit + EBITMAP_SIZE) {
-+			struct ebitmap_node *_n = kzalloc(sizeof(*_n), GFP_KERNEL);
++			struct ebitmap_node *_n;
++			_n = kzalloc(sizeof(*_n), GFP_KERNEL);
 +			if (!_n) {
-+				printk(KERN_ERR "security: ebitmap: out of memory\n");
++				printk(KERN_ERR
++				       "security: ebitmap: out of memory\n");
 +				rc = -ENOMEM;
 +				goto bad;
 +			}
-+			_n->startbit = startbit - (startbit % EBITMAP_SIZE); /* round down */
++			/* round down */
++			_n->startbit = startbit - (startbit % EBITMAP_SIZE);
 +			if (n) {
 +				n->next = _n;
 +			} else {
@@ -417,17 +441,18 @@
  	if (!rc)
  		rc = -EINVAL;
 diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
-index 1270e34..417053e 100644
+index 1270e34..e38a327 100644
 --- a/security/selinux/ss/ebitmap.h
 +++ b/security/selinux/ss/ebitmap.h
-@@ -16,14 +16,15 @@
+@@ -16,14 +16,16 @@

  #include <net/netlabel.h>

 -#define MAPTYPE u64			/* portion of bitmap in each node */
 -#define MAPSIZE (sizeof(MAPTYPE) * 8)	/* number of bits in node bitmap */
 -#define MAPBIT  1ULL			/* a bit in the node bitmap */
-+#define EBITMAP_UNIT_NUMS	((32 - sizeof(void *) - sizeof(u32)) / sizeof(unsigned long))
++#define EBITMAP_UNIT_NUMS	((32 - sizeof(void *) - sizeof(u32))	\
++					/ sizeof(unsigned long))
 +#define EBITMAP_UNIT_SIZE	BITS_PER_LONG
 +#define EBITMAP_SIZE		(EBITMAP_UNIT_NUMS * EBITMAP_UNIT_SIZE)
 +#define EBITMAP_BIT		1ULL
@@ -441,7 +466,7 @@
  };

  struct ebitmap {
-@@ -34,11 +35,17 @@ struct ebitmap {
+@@ -34,11 +36,17 @@ struct ebitmap {
  #define ebitmap_length(e) ((e)->highbit)
  #define ebitmap_startbit(e) ((e)->node ? (e)->node->startbit : 0)

@@ -463,7 +488,7 @@
  }

  static inline void ebitmap_init(struct ebitmap *e)
-@@ -46,28 +53,65 @@ static inline void ebitmap_init(struct ebitmap *e)
+@@ -46,28 +54,65 @@ static inline void ebitmap_init(struct ebitmap *e)
  	memset(e, 0, sizeof(*e));
  }

@@ -542,56 +567,67 @@
  int ebitmap_cmp(struct ebitmap *e1, struct ebitmap *e2);
  int ebitmap_cpy(struct ebitmap *dst, struct ebitmap *src);
 diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
-index 4a8bab2..27ef5eb 100644
+index 4a8bab2..9a11dea 100644
 --- a/security/selinux/ss/mls.c
 +++ b/security/selinux/ss/mls.c
-@@ -34,7 +34,7 @@
+@@ -34,7 +34,9 @@
   */
  int mls_compute_context_len(struct context * context)
  {
 -	int i, l, len, range;
 +	int i, l, len, head, prev;
++	char *nm;
++	struct ebitmap *e;
  	struct ebitmap_node *node;

  	if (!selinux_mls_enabled)
-@@ -42,31 +42,26 @@ int mls_compute_context_len(struct context * context)
+@@ -42,31 +44,33 @@ int mls_compute_context_len(struct context * context)

  	len = 1; /* for the beginning ":" */
  	for (l = 0; l < 2; l++) {
 -		range = 0;
- 		len += strlen(policydb.p_sens_val_to_name[context->range.level[l].sens - 1]);
-
+-		len += strlen(policydb.p_sens_val_to_name[context->range.level[l].sens - 1]);
+-
 -		ebitmap_for_each_bit(&context->range.level[l].cat, node, i) {
 -			if (ebitmap_node_get_bit(node, i)) {
 -				if (range) {
 -					range++;
 -					continue;
 -				}
--
-+		/* categories */
-+		head = -2;
-+		prev = -2;
-+		ebitmap_for_each_positive_bit(&context->range.level[l].cat, node, i) {
-+			if (i - prev > 1) {
-+				/* one or more negative bits are skipped, at least. */
-+				if (head != prev)
-+					len += strlen(policydb.p_cat_val_to_name[prev]) + 1;
- 				len += strlen(policydb.p_cat_val_to_name[i]) + 1;
++		int index_sens = context->range.level[l].sens;
++		len += strlen(policydb.p_sens_val_to_name[index_sens - 1]);
+
+-				len += strlen(policydb.p_cat_val_to_name[i]) + 1;
 -				range++;
 -			} else {
 -				if (range > 1)
 -					len += strlen(policydb.p_cat_val_to_name[i - 1]) + 1;
 -				range = 0;
++		/* categories */
++		head = -2;
++		prev = -2;
++		e = &context->range.level[l].cat;
++		ebitmap_for_each_positive_bit(e, node, i) {
++			if (i - prev > 1) {
++				/* one or more negative bits are skipped */
++				if (head != prev) {
++					nm = policydb.p_cat_val_to_name[prev];
++					len += strlen(nm) + 1;
++				}
++				nm = policydb.p_cat_val_to_name[i];
++				len += strlen(nm) + 1;
 +				head = i;
  			}
 +			prev = i;
++		}
++		if (prev != head) {
++			nm = policydb.p_cat_val_to_name[prev];
++			len += strlen(nm) + 1;
  		}
 -		/* Handle case where last category is the end of range */
 -		if (range > 1)
 -			len += strlen(policydb.p_cat_val_to_name[i - 1]) + 1;
 -
-+		if (prev != head)
-+			len += strlen(policydb.p_cat_val_to_name[prev]) + 1;
  		if (l == 0) {
  			if (mls_level_eq(&context->range.level[0],
 -			                 &context->range.level[1]))
@@ -599,16 +635,19 @@
  				break;
  			else
  				len++;
-@@ -85,7 +80,7 @@ void mls_sid_to_context(struct context *context,
+@@ -84,8 +88,9 @@ int mls_compute_context_len(struct context * context)
+ void mls_sid_to_context(struct context *context,
                          char **scontext)
  {
- 	char *scontextp;
+-	char *scontextp;
 -	int i, l, range, wrote_sep;
++	char *scontextp, *nm;
 +	int i, l, head, prev;
++	struct ebitmap *e;
  	struct ebitmap_node *node;

  	if (!selinux_mls_enabled)
-@@ -97,61 +92,50 @@ void mls_sid_to_context(struct context *context,
+@@ -97,61 +102,54 @@ void mls_sid_to_context(struct context *context,
  	scontextp++;

  	for (l = 0; l < 2; l++) {
@@ -640,9 +679,10 @@
 -					if (range > 2)
 +		head = -2;
 +		prev = -2;
-+		ebitmap_for_each_positive_bit(&context->range.level[l].cat, node, i) {
++		e = &context->range.level[l].cat;
++		ebitmap_for_each_positive_bit(e, node, i) {
 +			if (i - prev > 1) {
-+				/* one or more negative bits are skipped, at least. */
++				/* one or more negative bits are skipped */
 +				if (prev != head) {
 +					if (prev - head > 1)
  						*scontextp++ = '.';
@@ -651,16 +691,18 @@
 -
 -					strcpy(scontextp, policydb.p_cat_val_to_name[i - 1]);
 -					scontextp += strlen(policydb.p_cat_val_to_name[i - 1]);
-+					strcpy(scontextp, policydb.p_cat_val_to_name[prev]);
-+					scontextp += strlen(scontextp);
++					nm = policydb.p_cat_val_to_name[prev];
++					strcpy(scontextp, nm);
++					scontextp += strlen(nm);
  				}
 -				range = 0;
 +				if (prev < 0)
 +					*scontextp++ = ':';
 +				else
 +					*scontextp++ = ',';
-+				strcpy(scontextp, policydb.p_cat_val_to_name[i]);
-+				scontextp += strlen(scontextp);
++				nm = policydb.p_cat_val_to_name[i];
++				strcpy(scontextp, nm);
++				scontextp += strlen(nm);
 +				head = i;
  			}
 +			prev = i;
@@ -677,8 +719,9 @@
 -
 -			strcpy(scontextp, policydb.p_cat_val_to_name[i - 1]);
 -			scontextp += strlen(policydb.p_cat_val_to_name[i - 1]);
-+			strcpy(scontextp, policydb.p_cat_val_to_name[prev]);
-+			scontextp += strlen(scontextp);
++			nm = policydb.p_cat_val_to_name[prev];
++			strcpy(scontextp, nm);
++			scontextp += strlen(nm);
  		}

  		if (l == 0) {
@@ -694,7 +737,7 @@
  		}
  	}

-@@ -190,17 +174,15 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
+@@ -190,17 +188,15 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
  		if (!levdatum)
  			return 0;

@@ -721,7 +764,7 @@
  		}
  	}

-@@ -485,18 +467,16 @@ int mls_convert_context(struct policydb *oldp,
+@@ -485,18 +481,16 @@ int mls_convert_context(struct policydb *oldp,
  		c->range.level[l].sens = levdatum->level->sens;

  		ebitmap_init(&bitmap);

-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.