[PATCH 0/4] SELinux: Fine-tuning for some function implementations

[PATCH 0/4] SELinux: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Aug 2017 16:25:43 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Delete eight unnecessary variable assignments
  Adjust jump targets in ebitmap_read()
  Delete an unnecessary return statement in ebitmap_destroy()
  Adjust five checks for null pointers

 security/selinux/selinuxfs.c   |  1 -
 security/selinux/ss/avtab.c    |  2 --
 security/selinux/ss/ebitmap.c  | 53 +++++++++++++++++++-----------------------
 security/selinux/ss/policydb.c | 10 ++------
 4 files changed, 26 insertions(+), 40 deletions(-)

-- 
2.14.0

[PATCH 1/4] selinux: Delete eight unnecessary variable assignments

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Aug 2017 14:17:48 +0200

One local variable was reset to zero at the end of these functions.
This value will also be set by a previous call of a function if it was
executed successfully. Thus omit an extra assignment there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/selinuxfs.c   |  1 -
 security/selinux/ss/avtab.c    |  2 --
 security/selinux/ss/ebitmap.c  |  4 +---
 security/selinux/ss/policydb.c | 10 ++--------
 4 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 00eed842c491..7565c312a198 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1724,7 +1724,6 @@ static int sel_make_classes(void)
 		if (rc)
 			goto out;
 	}
-	rc = 0;
 out:
 	for (i = 0; i < nclasses; i++)
 		kfree(classes[i]);
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 3628d3a868b6..a8218905e286 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -587,8 +587,6 @@ int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
 			goto bad;
 		}
 	}
-
-	rc = 0;
 out:
 	return rc;
 
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index ad38299164c3..ccf372db689c 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -373,7 +373,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 
 	if (!e->highbit) {
 		e->node = NULL;
-		goto ok;
+		goto out;
 	}
 
 	if (e->highbit && !count)
@@ -436,8 +436,6 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 			map = EBITMAP_SHIFT_UNIT_SIZE(map);
 		}
 	}
-ok:
-	rc = 0;
 out:
 	return rc;
 bad:
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index aa6500abb178..a2356fc263c6 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -599,7 +599,6 @@ static int policydb_index(struct policydb *p)
 		if (rc)
 			goto out;
 	}
-	rc = 0;
 out:
 	return rc;
 }
@@ -903,10 +902,10 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
 
 	head = p->ocontexts[OCON_ISID];
 	for (c = head; c; c = c->next) {
-		rc = -EINVAL;
 		if (!c->context[0].user) {
 			printk(KERN_ERR "SELinux:  SID %s was never defined.\n",
 				c->u.name);
+			rc = -EINVAL;
 			goto out;
 		}
 
@@ -917,7 +916,6 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
 			goto out;
 		}
 	}
-	rc = 0;
 out:
 	return rc;
 }
@@ -1074,13 +1072,12 @@ static int context_read_and_validate(struct context *c,
 		}
 	}
 
-	rc = -EINVAL;
 	if (!policydb_context_isvalid(p, c)) {
 		printk(KERN_ERR "SELinux:  invalid security context\n");
 		context_destroy(c);
+		rc = -EINVAL;
 		goto out;
 	}
-	rc = 0;
 out:
 	return rc;
 }
@@ -1900,7 +1897,6 @@ static int range_read(struct policydb *p, void *fp)
 		r = NULL;
 	}
 	hash_eval(p->range_tr, "rangetr");
-	rc = 0;
 out:
 	kfree(rt);
 	kfree(r);
@@ -2550,8 +2546,6 @@ int policydb_read(struct policydb *p, void *fp)
 	rc = policydb_bounds_sanity_check(p);
 	if (rc)
 		goto bad;
-
-	rc = 0;
 out:
 	return rc;
 bad:
-- 
2.14.0

Re: [PATCH 1/4] selinux: Delete eight unnecessary variable assignments

[Serge E. Hallyn]

Quoting SF Markus Elfring (elfring@users.sourceforge.net):
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Aug 2017 14:17:48 +0200
> 
> One local variable was reset to zero at the end of these functions.
> This value will also be set by a previous call of a function if it was
> executed successfully. Thus omit an extra assignment there.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Serge Hallyn <serge@hallyn.com>

> ---
>  security/selinux/selinuxfs.c   |  1 -
>  security/selinux/ss/avtab.c    |  2 --
>  security/selinux/ss/ebitmap.c  |  4 +---
>  security/selinux/ss/policydb.c | 10 ++--------
>  4 files changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 00eed842c491..7565c312a198 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1724,7 +1724,6 @@ static int sel_make_classes(void)
>  		if (rc)
>  			goto out;
>  	}
> -	rc = 0;
>  out:
>  	for (i = 0; i < nclasses; i++)
>  		kfree(classes[i]);
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 3628d3a868b6..a8218905e286 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -587,8 +587,6 @@ int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
>  			goto bad;
>  		}
>  	}
> -
> -	rc = 0;
>  out:
>  	return rc;
>  
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index ad38299164c3..ccf372db689c 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -373,7 +373,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>  
>  	if (!e->highbit) {
>  		e->node = NULL;
> -		goto ok;
> +		goto out;
>  	}
>  
>  	if (e->highbit && !count)
> @@ -436,8 +436,6 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>  			map = EBITMAP_SHIFT_UNIT_SIZE(map);
>  		}
>  	}
> -ok:
> -	rc = 0;
>  out:
>  	return rc;
>  bad:
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index aa6500abb178..a2356fc263c6 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -599,7 +599,6 @@ static int policydb_index(struct policydb *p)
>  		if (rc)
>  			goto out;
>  	}
> -	rc = 0;
>  out:
>  	return rc;
>  }
> @@ -903,10 +902,10 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
>  
>  	head = p->ocontexts[OCON_ISID];
>  	for (c = head; c; c = c->next) {
> -		rc = -EINVAL;
>  		if (!c->context[0].user) {
>  			printk(KERN_ERR "SELinux:  SID %s was never defined.\n",
>  				c->u.name);
> +			rc = -EINVAL;
>  			goto out;
>  		}
>  
> @@ -917,7 +916,6 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
>  			goto out;
>  		}
>  	}
> -	rc = 0;
>  out:
>  	return rc;
>  }
> @@ -1074,13 +1072,12 @@ static int context_read_and_validate(struct context *c,
>  		}
>  	}
>  
> -	rc = -EINVAL;
>  	if (!policydb_context_isvalid(p, c)) {
>  		printk(KERN_ERR "SELinux:  invalid security context\n");
>  		context_destroy(c);
> +		rc = -EINVAL;
>  		goto out;
>  	}
> -	rc = 0;
>  out:
>  	return rc;
>  }
> @@ -1900,7 +1897,6 @@ static int range_read(struct policydb *p, void *fp)
>  		r = NULL;
>  	}
>  	hash_eval(p->range_tr, "rangetr");
> -	rc = 0;
>  out:
>  	kfree(rt);
>  	kfree(r);
> @@ -2550,8 +2546,6 @@ int policydb_read(struct policydb *p, void *fp)
>  	rc = policydb_bounds_sanity_check(p);
>  	if (rc)
>  		goto bad;
> -
> -	rc = 0;
>  out:
>  	return rc;
>  bad:
> -- 
> 2.14.0

Re: [PATCH 1/4] selinux: Delete eight unnecessary variable assignments

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serge@hallyn.com):
> Quoting SF Markus Elfring (elfring@users.sourceforge.net):
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 13 Aug 2017 14:17:48 +0200
> > 
> > One local variable was reset to zero at the end of these functions.
> > This value will also be set by a previous call of a function if it was
> > executed successfully. Thus omit an extra assignment there.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Acked-by: Serge Hallyn <serge@hallyn.com>

BTW this does add a burden to the maintainers as it makes the
code a bit more fragile in the face of future changes.  So my
ack means it looks ok, but if the maintainers don't want to take
it I absolutely understand.

[PATCH 2/4] selinux: Adjust jump targets in ebitmap_read()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Aug 2017 15:21:43 +0200

Adjust jump targets so that the function implementation becomes smaller.

* Move an error message so that it is present only once here.

* Avoid another check for the local variable "rc" at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/ebitmap.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index ccf372db689c..03581d7ef817 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -350,21 +350,20 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 	__le32 buf[3];
 	int rc, i;
 
-	ebitmap_init(e);
-
 	rc = next_entry(buf, fp, sizeof buf);
 	if (rc < 0)
 		goto out;
 
-	mapunit = le32_to_cpu(buf[0]);
+	ebitmap_init(e);
 	e->highbit = le32_to_cpu(buf[1]);
 	count = le32_to_cpu(buf[2]);
+	mapunit = le32_to_cpu(buf[0]);
 
 	if (mapunit != BITS_PER_U64) {
 		printk(KERN_ERR "SELinux: ebitmap: map size %u does not "
 		       "match my size %zd (high bit was %d)\n",
 		       mapunit, BITS_PER_U64, e->highbit);
-		goto bad;
+		goto destroy_bitmap;
 	}
 
 	/* round up e->highbit */
@@ -377,27 +376,26 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 	}
 
 	if (e->highbit && !count)
-		goto bad;
+		goto destroy_bitmap;
 
 	for (i = 0; i < count; i++) {
 		rc = next_entry(&startbit, fp, sizeof(u32));
-		if (rc < 0) {
-			printk(KERN_ERR "SELinux: ebitmap: truncated map\n");
-			goto bad;
-		}
+		if (rc)
+			goto report_truncated_map;
+
 		startbit = le32_to_cpu(startbit);
 
 		if (startbit & (mapunit - 1)) {
 			printk(KERN_ERR "SELinux: ebitmap start bit (%d) is "
 			       "not a multiple of the map unit size (%u)\n",
 			       startbit, mapunit);
-			goto bad;
+			goto destroy_bitmap;
 		}
 		if (startbit > e->highbit - mapunit) {
 			printk(KERN_ERR "SELinux: ebitmap start bit (%d) is "
 			       "beyond the end of the bitmap (%u)\n",
 			       startbit, (e->highbit - mapunit));
-			goto bad;
+			goto destroy_bitmap;
 		}
 
 		if (!n || startbit >= n->startbit + EBITMAP_SIZE) {
@@ -407,7 +405,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 				printk(KERN_ERR
 				       "SELinux: ebitmap: out of memory\n");
 				rc = -ENOMEM;
-				goto bad;
+				goto destroy_bitmap;
 			}
 			/* round down */
 			tmp->startbit = startbit - (startbit % EBITMAP_SIZE);
@@ -420,14 +418,13 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 			printk(KERN_ERR "SELinux: ebitmap: start bit %d"
 			       " comes after start bit %d\n",
 			       startbit, n->startbit);
-			goto bad;
+			goto destroy_bitmap;
 		}
 
 		rc = next_entry(&map, fp, sizeof(u64));
-		if (rc < 0) {
-			printk(KERN_ERR "SELinux: ebitmap: truncated map\n");
-			goto bad;
-		}
+		if (rc)
+			goto report_truncated_map;
+
 		map = le64_to_cpu(map);
 
 		index = (startbit - n->startbit) / EBITMAP_UNIT_SIZE;
@@ -438,9 +435,10 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 	}
 out:
 	return rc;
-bad:
-	if (!rc)
-		rc = -EINVAL;
+report_truncated_map:
+	printk(KERN_ERR "SELinux: ebitmap: truncated map\n");
+	rc = -EINVAL;
+destroy_bitmap:
 	ebitmap_destroy(e);
 	goto out;
 }
-- 
2.14.0

Re: [PATCH 2/4] selinux: Adjust jump targets in ebitmap_read()

[Serge E. Hallyn]

Quoting SF Markus Elfring (elfring@users.sourceforge.net):
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Aug 2017 15:21:43 +0200
> 
> Adjust jump targets so that the function implementation becomes smaller.

By how much?

[PATCH 3/4] selinux: Delete an unnecessary return statement in ebitmap_destroy()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Aug 2017 15:50:26 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: void function return statements are not generally useful

Thus remove such a statement in the affected function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/ebitmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 03581d7ef817..697bd748760a 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -339,7 +339,6 @@ void ebitmap_destroy(struct ebitmap *e)
 
 	e->highbit = 0;
 	e->node = NULL;
-	return;
 }
 
 int ebitmap_read(struct ebitmap *e, void *fp)
-- 
2.14.0

[PATCH 4/4] selinux: Adjust five checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Aug 2017 16:16:05 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written …

Thus fix affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/ebitmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 697bd748760a..c778135989f5 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -96,12 +96,12 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
 	unsigned int iter;
 	int rc;
 
-	if (e_iter = NULL) {
+	if (!e_iter) {
 		*catmap = NULL;
 		return 0;
 	}
 
-	if (*catmap != NULL)
+	if (*catmap)
 		netlbl_catmap_free(*catmap);
 	*catmap = NULL;
 
@@ -161,14 +161,14 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
 			continue;
 		}
 
-		if (e_iter = NULL ||
+		if (!e_iter ||
 		    offset >= e_iter->startbit + EBITMAP_SIZE) {
 			e_prev = e_iter;
 			e_iter = kmem_cache_zalloc(ebitmap_node_cachep, GFP_ATOMIC);
-			if (e_iter = NULL)
+			if (!e_iter)
 				goto netlbl_import_failure;
 			e_iter->startbit = offset - (offset % EBITMAP_SIZE);
-			if (e_prev = NULL)
+			if (!e_prev)
 				ebmap->node = e_iter;
 			else
 				e_prev->next = e_iter;
-- 
2.14.0

Re: [PATCH 4/4] selinux: Adjust five checks for null pointers

[Serge E. Hallyn]

Quoting SF Markus Elfring (elfring@users.sourceforge.net):
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Aug 2017 16:16:05 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written …

...  could be written all sorts of ways.  But what's the advantage of this?
Personally I find "x = NULL" easier to read, and AFAIK psychology backs me
up on the idea that negation is harder on the brain and worth avoiding when
possible.

> Thus fix affected source code places.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/ebitmap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 697bd748760a..c778135989f5 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -96,12 +96,12 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
>  	unsigned int iter;
>  	int rc;
>  
> -	if (e_iter = NULL) {
> +	if (!e_iter) {
>  		*catmap = NULL;
>  		return 0;
>  	}
>  
> -	if (*catmap != NULL)
> +	if (*catmap)
>  		netlbl_catmap_free(*catmap);
>  	*catmap = NULL;
>  
> @@ -161,14 +161,14 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
>  			continue;
>  		}
>  
> -		if (e_iter = NULL ||
> +		if (!e_iter ||
>  		    offset >= e_iter->startbit + EBITMAP_SIZE) {
>  			e_prev = e_iter;
>  			e_iter = kmem_cache_zalloc(ebitmap_node_cachep, GFP_ATOMIC);
> -			if (e_iter = NULL)
> +			if (!e_iter)
>  				goto netlbl_import_failure;
>  			e_iter->startbit = offset - (offset % EBITMAP_SIZE);
> -			if (e_prev = NULL)
> +			if (!e_prev)
>  				ebmap->node = e_iter;
>  			else
>  				e_prev->next = e_iter;
> -- 
> 2.14.0

Re: [PATCH 4/4] selinux: Adjust five checks for null pointers

[Yanhao Mo]

[-- Attachment #1: Type: text/plain, Size: 217 bytes --]

On Sun, Aug 13, 2017 at 04:50:07PM +0200, SF Markus Elfring wrote:
> 
> Comparison to NULL could be written …
> 
> Thus fix affected source code places.

I think this is just a matter of personal preference.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] SELinux: Fine-tuning for some function implementations

[Paul Moore]

On Sun, Aug 13, 2017 at 10:43 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Aug 2017 16:25:43 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
>   Delete eight unnecessary variable assignments
>   Adjust jump targets in ebitmap_read()
>   Delete an unnecessary return statement in ebitmap_destroy()
>   Adjust five checks for null pointers

Hi Markus,

I merged many of your patches in the past because I wanted to help
encourage your involvement, but you will remember I mentioned at the
time that I generally dislike merging these trivial, and often
style-only, patches.  I believe I even pointed you in the direction of
our SELinux kernel issue tracker for a list of areas where you could
help contribute in a meaningful way (link below).  My earlier comments
still apply; if you want to continue to contribute to SELinux in the
kernel, please focus your attention on more meaningful changes.  I am
not going to merge any of these patches.

* https://github.com/SELinuxProject/selinux-kernel/issues

-- 
paul moore
www.paul-moore.com