From: Markus Elfring <[email protected]>
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
Quoting SF Markus Elfring ([email protected]):
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
Quoting SF Markus Elfring ([email protected]):
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
> ---
> 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
Quoting Serge E. Hallyn ([email protected]):
> Quoting SF Markus Elfring ([email protected]):
> > From: Markus Elfring <[email protected]>
> > 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 <[email protected]>
>
> Acked-by: Serge Hallyn <[email protected]>
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.
Quoting SF Markus Elfring ([email protected]):
> From: Markus Elfring <[email protected]>
> Date: Sun, 13 Aug 2017 15:21:43 +0200
>
> Adjust jump targets so that the function implementation becomes smaller.
By how much?
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.
On Sun, Aug 13, 2017 at 10:43 AM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> 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
http://www.paul-moore.com