Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1559248 > unrolled thread

[PATCH 00/46] SELinux: Fine-tuning for several function implementations

Started bySF Markus Elfring <elfring@users.sourceforge.net>
First post2017-01-15 16:00 +0100
Last post2017-01-16 19:40 +0100
Articles 16 on this page of 56 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/46] SELinux: Fine-tuning for several function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:00 +0100
    [PATCH 02/46] selinux: Delete an unnecessary return statement in  cond_compute_av() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:00 +0100
    [PATCH 01/46] selinux: Use kmalloc_array() in  cond_init_bool_indexes() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:00 +0100
    [PATCH 04/46] selinux: Use kmalloc_array() in hashtab_create() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 05/46] selinux: Adjust four checks for null pointers SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 07/46] selinux: Delete unnecessary variable assignments in  policydb_index() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
      Re: [PATCH 07/46] selinux: Delete unnecessary variable assignments in  policydb_index() Casey Schaufler <casey@schaufler-ca.com> - 2017-01-17 17:40 +0100
    [PATCH 09/46] selinux: Delete an error message for a failed memory  allocation in policydb_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 08/46] selinux: Delete an unnecessary return statement in  policydb_destroy() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 03/46] selinux: Improve size determinations in four functions SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 06/46] selinux: Use kcalloc() in policydb_index() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:10 +0100
    [PATCH 18/46] selinux: One function call less in  filename_trans_read() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 17/46] selinux: Delete an unnecessary variable assignment in  filename_trans_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 11/46] selinux: Return directly after a failed next_entry() in  genfs_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 15/46] selinux: One check and function call less in  genfs_read() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
      Re: [PATCH 15/46] selinux: One check and function call less in  genfs_read() after error detection Casey Schaufler <casey@schaufler-ca.com> - 2017-01-17 17:40 +0100
        Re: [PATCH 15/46] selinux: One check and function call less in  genfs_read() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-17 17:40 +0100
          Re: [PATCH 15/46] selinux: One check and function call less in  genfs_read() after error detection Casey Schaufler <casey@schaufler-ca.com> - 2017-01-17 19:00 +0100
    [PATCH 16/46] selinux: Move two assignments for the variable "rc" in  filename_trans_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 13/46] selinux: Move four assignments for the variable "rc" in  genfs_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 14/46] selinux: One function call less in genfs_read() after  null pointer detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 19/46] selinux: Return directly after a failed next_entry() in  range_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 10/46] selinux: Move some assignments for the variable "rc" in  policydb_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 12/46] selinux: Move assignments for two pointers in  genfs_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:20 +0100
    [PATCH 28/46] selinux: Return directly after a failed kzalloc() in  user_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 20/46] selinux: Move four assignments for the variable "rc" in  range_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 24/46] selinux: Return directly after a failed kzalloc() in  cat_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 21/46] selinux: Two function calls less in range_read() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
      Re: [PATCH 21/46] selinux: Two function calls less in range_read()  after error detection Casey Schaufler <casey@schaufler-ca.com> - 2017-01-17 17:40 +0100
    [PATCH 23/46] selinux: Move an assignment for a pointer in  range_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 29/46] selinux: Return directly after a failed kzalloc() in  type_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 25/46] selinux: Return directly after a failed kzalloc() in  sens_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 26/46] selinux: Improve another size determination in  sens_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 27/46] selinux: Move an assignment for the variable "rc" in  sens_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 22/46] selinux: Delete an unnecessary variable initialisation  in range_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:30 +0100
    [PATCH 35/46] selinux: Return directly after a failed kzalloc() in  perm_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 32/46] selinux: Return directly after a failed kzalloc() in  class_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 39/46] selinux: Move two assignments for the variable "rc" in  ocontext_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 33/46] selinux: Move an assignment for the variable "rc" in  class_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 37/46] selinux: Move an assignment for the variable "rc" in  policydb_load_isids() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 38/46] selinux: One function call less in five functions after  null pointer detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 30/46] selinux: Return directly after a failed kzalloc() in  role_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 31/46] selinux: Move an assignment for the variable "rc" in  role_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 36/46] selinux: Move an assignment for the variable "rc" in  mls_read_range_helper() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 34/46] selinux: Return directly after a failed kzalloc() in  common_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:40 +0100
    [PATCH 45/46] selinux: Use common error handling code in  sidtab_insert() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
      Re: [PATCH 45/46] selinux: Use common error handling code in  sidtab_insert() Casey Schaufler <casey@schaufler-ca.com> - 2017-01-17 19:10 +0100
    [PATCH 44/46] selinux: Adjust two checks for null pointers SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    [PATCH 40/46] selinux: Return directly after a failed kzalloc() in  roles_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    [PATCH 46/46] selinuxfs: Use seq_puts() in sel_avc_stats_seq_show() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    [PATCH 43/46] selinux: Use kmalloc_array() in sidtab_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    [PATCH 42/46] selinux: One function call less in roles_init() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    [PATCH 41/46] selinux: Move two assignments for the variable "rc" in  roles_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-15 16:50 +0100
    Re: [PATCH 00/46] SELinux: Fine-tuning for several function  implementations Eric Paris <eparis@redhat.com> - 2017-01-16 16:30 +0100
      Re: [PATCH 00/46] SELinux: Fine-tuning for several function implementations Paul Moore <paul@paul-moore.com> - 2017-01-16 17:50 +0100
      Re: SELinux: Checking source code positions for the setting of error  codes SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-16 19:40 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1559286 — [PATCH 38/46] selinux: One function call less in five functions after null pointer detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:40 +0100
Subject[PATCH 38/46] selinux: One function call less in five functions after null pointer detection
Message-ID<sZV4u-4aR-15@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 11:28:02 +0100

Call the function "kfree" at the end only after it was determined
that the passed parameter contained a non-null pointer.

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

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index faa6ecc2450d..88730b372277 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -620,8 +620,8 @@ static int common_destroy(void *key, void *datum, void *p)
 		comdatum = datum;
 		hashtab_map(comdatum->permissions.table, perm_destroy, NULL);
 		hashtab_destroy(comdatum->permissions.table);
+		kfree(datum);
 	}
-	kfree(datum);
 	return 0;
 }
 
@@ -675,8 +675,8 @@ static int cls_destroy(void *key, void *datum, void *p)
 			kfree(ctemp);
 		}
 		kfree(cladatum->comkey);
+		kfree(datum);
 	}
-	kfree(datum);
 	return 0;
 }
 
@@ -689,8 +689,8 @@ static int role_destroy(void *key, void *datum, void *p)
 		role = datum;
 		ebitmap_destroy(&role->dominates);
 		ebitmap_destroy(&role->types);
+		kfree(datum);
 	}
-	kfree(datum);
 	return 0;
 }
 
@@ -712,8 +712,8 @@ static int user_destroy(void *key, void *datum, void *p)
 		ebitmap_destroy(&usrdatum->range.level[0].cat);
 		ebitmap_destroy(&usrdatum->range.level[1].cat);
 		ebitmap_destroy(&usrdatum->dfltlevel.cat);
+		kfree(datum);
 	}
-	kfree(datum);
 	return 0;
 }
 
@@ -726,8 +726,8 @@ static int sens_destroy(void *key, void *datum, void *p)
 		levdatum = datum;
 		ebitmap_destroy(&levdatum->level->cat);
 		kfree(levdatum->level);
+		kfree(datum);
 	}
-	kfree(datum);
 	return 0;
 }
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559287 — [PATCH 30/46] selinux: Return directly after a failed kzalloc() in role_read()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:40 +0100
Subject[PATCH 30/46] selinux: Return directly after a failed kzalloc() in role_read()
Message-ID<sZV4u-4aR-23@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 14 Jan 2017 22:20:25 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/policydb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 662139365449..34b670227c4d 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1410,10 +1410,9 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp)
 	__le32 buf[3];
 	u32 len;
 
-	rc = -ENOMEM;
 	role = kzalloc(sizeof(*role), GFP_KERNEL);
 	if (!role)
-		goto bad;
+		return -ENOMEM;
 
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		to_read = 3;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559288 — [PATCH 31/46] selinux: Move an assignment for the variable "rc" in role_read()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:40 +0100
Subject[PATCH 31/46] selinux: Move an assignment for the variable "rc" in role_read()
Message-ID<sZV4u-4aR-31@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 14 Jan 2017 22:25:20 +0100

A local variable was set to an error code in one case before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

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

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 34b670227c4d..49fc5d8990e9 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1439,10 +1439,10 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp)
 		goto bad;
 
 	if (strcmp(key, OBJECT_R) == 0) {
-		rc = -EINVAL;
 		if (role->value != OBJECT_R_VAL) {
 			printk(KERN_ERR "SELinux: Role %s has wrong value %d\n",
 			       OBJECT_R, role->value);
+			rc = -EINVAL;
 			goto bad;
 		}
 		rc = 0;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559289 — [PATCH 36/46] selinux: Move an assignment for the variable "rc" in mls_read_range_helper()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:40 +0100
Subject[PATCH 36/46] selinux: Move an assignment for the variable "rc" in mls_read_range_helper()
Message-ID<sZV4u-4aR-21@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 11:22:23 +0100

A local variable was set to an error code in one case before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

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

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 9b595f2e0d9f..7cf635c650dc 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -996,10 +996,10 @@ static int mls_read_range_helper(struct mls_range *r, void *fp)
 	if (rc)
 		goto out;
 
-	rc = -EINVAL;
 	items = le32_to_cpu(buf[0]);
 	if (items > ARRAY_SIZE(buf)) {
 		printk(KERN_ERR "SELinux: mls:  range overflow\n");
+		rc = -EINVAL;
 		goto out;
 	}
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559290 — [PATCH 34/46] selinux: Return directly after a failed kzalloc() in common_read()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:40 +0100
Subject[PATCH 34/46] selinux: Return directly after a failed kzalloc() in common_read()
Message-ID<sZV4u-4aR-25@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 11:15:19 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/policydb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 9035e5329ceb..551685283399 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1150,10 +1150,9 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp)
 	u32 len, nel;
 	int i, rc;
 
-	rc = -ENOMEM;
 	comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
 	if (!comdatum)
-		goto bad;
+		return -ENOMEM;
 
 	rc = next_entry(buf, fp, sizeof buf);
 	if (rc)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559291 — [PATCH 45/46] selinux: Use common error handling code in sidtab_insert()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 45/46] selinux: Use common error handling code in sidtab_insert()
Message-ID<sZVe9-4eg-1@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 13:45:45 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

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

diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index f6915f257486..4130f882808c 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -35,10 +35,8 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	int hvalue, rc = 0;
 	struct sidtab_node *prev, *cur, *newnode;
 
-	if (!s) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!s)
+		goto failure_indication;
 
 	hvalue = SIDTAB_HASH(sid);
 	prev = NULL;
@@ -54,15 +52,12 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	}
 
 	newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
-	if (!newnode) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!newnode)
+		goto failure_indication;
 	newnode->sid = sid;
 	if (context_cpy(&newnode->context, context)) {
 		kfree(newnode);
-		rc = -ENOMEM;
-		goto out;
+		goto failure_indication;
 	}
 
 	if (prev) {
@@ -80,6 +75,9 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 		s->next_sid = sid + 1;
 out:
 	return rc;
+failure_indication:
+	rc = -ENOMEM;
+	goto out;
 }
 
 static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1560880 — Re: [PATCH 45/46] selinux: Use common error handling code in sidtab_insert()

FromCasey Schaufler <casey@schaufler-ca.com>
Date2017-01-17 19:10 +0100
SubjectRe: [PATCH 45/46] selinux: Use common error handling code in sidtab_insert()
Message-ID<t0GmK-1VI-9@gated-at.bofh.it>
In reply to#1559291
On 1/15/2017 7:45 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 13:45:45 +0100
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index f6915f257486..4130f882808c 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -35,10 +35,8 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>  	int hvalue, rc = 0;
>  	struct sidtab_node *prev, *cur, *newnode;
>  
> -	if (!s) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> +	if (!s)
> +		goto failure_indication;
>  
>  	hvalue = SIDTAB_HASH(sid);
>  	prev = NULL;
> @@ -54,15 +52,12 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>  	}
>  
>  	newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
> -	if (!newnode) {
> -		rc = -ENOMEM;
> -		goto out;

Why not "return -ENOMEM;" ?

> -	}
> +	if (!newnode)
> +		goto failure_indication;
>  	newnode->sid = sid;
>  	if (context_cpy(&newnode->context, context)) {
>  		kfree(newnode);
> -		rc = -ENOMEM;
> -		goto out;
> +		goto failure_indication;

Again, "return -ENOMEM:"

>  	}
>  
>  	if (prev) {
> @@ -80,6 +75,9 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>  		s->next_sid = sid + 1;
>  out:
>  	return rc;
> +failure_indication:
> +	rc = -ENOMEM;
> +	goto out;

Backward gotos are horrible. Don't do this.

>  }
>  
>  static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)

[toc] | [prev] | [next] | [standalone]

#1559293 — [PATCH 44/46] selinux: Adjust two checks for null pointers

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 44/46] selinux: Adjust two checks for null pointers
Message-ID<sZVea-4eg-9@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 13:30:20 +0100
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/sidtab.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index c9533b21942b..f6915f257486 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -54,7 +54,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	}
 
 	newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
-	if (newnode == NULL) {
+	if (!newnode) {
 		rc = -ENOMEM;
 		goto out;
 	}
@@ -98,7 +98,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
 	if (force && cur && sid == cur->sid && cur->context.len)
 		return &cur->context;
 
-	if (cur == NULL || sid != cur->sid || cur->context.len) {
+	if (!cur || sid != cur->sid || cur->context.len) {
 		/* Remap invalid SIDs to the unlabeled SID. */
 		sid = SECINITSID_UNLABELED;
 		hvalue = SIDTAB_HASH(sid);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559294 — [PATCH 40/46] selinux: Return directly after a failed kzalloc() in roles_init()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 40/46] selinux: Return directly after a failed kzalloc() in roles_init()
Message-ID<sZVea-4eg-3@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 12:10:09 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/policydb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 8b9ed3f1b132..ccc146bfd4c2 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -178,10 +178,9 @@ static int roles_init(struct policydb *p)
 	int rc;
 	struct role_datum *role;
 
-	rc = -ENOMEM;
 	role = kzalloc(sizeof(*role), GFP_KERNEL);
 	if (!role)
-		goto out;
+		return -ENOMEM;
 
 	rc = -EINVAL;
 	role->value = ++p->p_roles.nprim;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559295 — [PATCH 46/46] selinuxfs: Use seq_puts() in sel_avc_stats_seq_show()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 46/46] selinuxfs: Use seq_puts() in sel_avc_stats_seq_show()
Message-ID<sZVea-4eg-15@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 14:04:53 +0100

A string which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

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

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 55345f84f17d..0d81ebf7ff23 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1445,10 +1445,10 @@ static int sel_avc_stats_seq_show(struct seq_file *seq, void *v)
 {
 	struct avc_cache_stats *st = v;
 
-	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "lookups hits misses allocations reclaims "
-			   "frees\n");
-	else {
+	if (v == SEQ_START_TOKEN) {
+		seq_puts(seq,
+			 "lookups hits misses allocations reclaims frees\n");
+	} else {
 		unsigned int lookups = st->lookups;
 		unsigned int misses = st->misses;
 		unsigned int hits = lookups - misses;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559297 — [PATCH 43/46] selinux: Use kmalloc_array() in sidtab_init()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 43/46] selinux: Use kmalloc_array() in sidtab_init()
Message-ID<sZVea-4eg-13@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 13:13:19 +0100

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

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

diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index 5840a35155fc..c9533b21942b 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -18,7 +18,7 @@ int sidtab_init(struct sidtab *s)
 {
 	int i;
 
-	s->htable = kmalloc(sizeof(*(s->htable)) * SIDTAB_SIZE, GFP_ATOMIC);
+	s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC);
 	if (!s->htable)
 		return -ENOMEM;
 	for (i = 0; i < SIDTAB_SIZE; i++)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559300 — [PATCH 42/46] selinux: One function call less in roles_init() after error detection

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 42/46] selinux: One function call less in roles_init() after error detection
Message-ID<sZVeb-4eg-25@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 12:56:54 +0100

The kfree() function was called in up to two cases by the
roles_init() function during error handling even if the passed variable
contained a null pointer.

* Adjust a jump target according to the Linux coding style convention.

* Delete an initialisation for the variable "key" at the beginning
  which became unnecessary with this refactoring.

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

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index be445abf047b..7dd5c6f7786f 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -174,7 +174,7 @@ static struct policydb_compat_info *policydb_lookup_compat(int version)
  */
 static int roles_init(struct policydb *p)
 {
-	char *key = NULL;
+	char *key;
 	int rc;
 	struct role_datum *role;
 
@@ -185,13 +185,13 @@ static int roles_init(struct policydb *p)
 	role->value = ++p->p_roles.nprim;
 	if (role->value != OBJECT_R_VAL) {
 		rc = -EINVAL;
-		goto out;
+		goto free_role;
 	}
 
 	key = kstrdup(OBJECT_R, GFP_KERNEL);
 	if (!key) {
 		rc = -ENOMEM;
-		goto out;
+		goto free_role;
 	}
 
 	rc = hashtab_insert(p->p_roles.table, key, role);
@@ -201,6 +201,7 @@ static int roles_init(struct policydb *p)
 	return 0;
 out:
 	kfree(key);
+free_role:
 	kfree(role);
 	return rc;
 }
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559301 — [PATCH 41/46] selinux: Move two assignments for the variable "rc" in roles_init()

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-15 16:50 +0100
Subject[PATCH 41/46] selinux: Move two assignments for the variable "rc" in roles_init()
Message-ID<sZVeb-4eg-23@gated-at.bofh.it>
In reply to#1559248
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Jan 2017 12:40:35 +0100

One local variable was set to an error code in two cases before
a concrete error situation was detected. Thus move the corresponding
assignments into if branches to indicate a software failure there.

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

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index ccc146bfd4c2..be445abf047b 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -182,15 +182,17 @@ static int roles_init(struct policydb *p)
 	if (!role)
 		return -ENOMEM;
 
-	rc = -EINVAL;
 	role->value = ++p->p_roles.nprim;
-	if (role->value != OBJECT_R_VAL)
+	if (role->value != OBJECT_R_VAL) {
+		rc = -EINVAL;
 		goto out;
+	}
 
-	rc = -ENOMEM;
 	key = kstrdup(OBJECT_R, GFP_KERNEL);
-	if (!key)
+	if (!key) {
+		rc = -ENOMEM;
 		goto out;
+	}
 
 	rc = hashtab_insert(p->p_roles.table, key, role);
 	if (rc)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1559834

FromEric Paris <eparis@redhat.com>
Date2017-01-16 16:30 +0100
Message-ID<t0hol-2dP-13@gated-at.bofh.it>
In reply to#1559248
<opinion from irrelevant person>

All of the patches look good to me except most of those which change
the handling of `rc=`. I have a personal style preference for

rc = -ENOMEM;
val = kalloc();
if (!val)
  goto err;

vs

val = kalloc();
if (!val) {
  rc = -ENOMEM;
  goto err;
}

because it saves 1 line and I think the compiler does the right/same
thing. If there is preference among the people active in selinux
developers (like I said, I'm now irrelevant) I guess they win.

But certainly a big +1 from me for the array allocation and sizeof()
changes.

-Eric

On Sun, 2017-01-15 at 15:55 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 15:15:14 +0100
> 
> Several update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (46):
>   Use kmalloc_array() in cond_init_bool_indexes()
>   Delete an unnecessary return statement in cond_compute_av()
>   Improve size determinations in four functions
>   Use kmalloc_array() in hashtab_create()
>   Adjust four checks for null pointers
>   Use kcalloc() in policydb_index()
>   Delete unnecessary variable assignments in policydb_index()
>   Delete an unnecessary return statement in policydb_destroy()
>   Delete an error message for a failed memory allocation in
> policydb_read()
>   Move some assignments for the variable "rc" in policydb_read()
>   Return directly after a failed next_entry() in genfs_read()
>   Move assignments for two pointers in genfs_read()
>   Move four assignments for the variable "rc" in genfs_read()
>   One function call less in genfs_read() after null pointer detection
>   One check and function call less in genfs_read() after error
> detection
>   Move two assignments for the variable "rc" in filename_trans_read()
>   Delete an unnecessary variable assignment in filename_trans_read()
>   One function call less in filename_trans_read() after error
> detection
>   Return directly after a failed next_entry() in range_read()
>   Move four assignments for the variable "rc" in range_read()
>   Two function calls less in range_read() after error detection
>   Delete an unnecessary variable initialisation in range_read()
>   Move an assignment for a pointer in range_read()
>   Return directly after a failed kzalloc() in cat_read()
>   Return directly after a failed kzalloc() in sens_read()
>   Improve another size determination in sens_read()
>   Move an assignment for the variable "rc" in sens_read()
>   Return directly after a failed kzalloc() in user_read()
>   Return directly after a failed kzalloc() in type_read()
>   Return directly after a failed kzalloc() in role_read()
>   Move an assignment for the variable "rc" in role_read()
>   Return directly after a failed kzalloc() in class_read()
>   Move an assignment for the variable "rc" in class_read()
>   Return directly after a failed kzalloc() in common_read()
>   Return directly after a failed kzalloc() in perm_read()
>   Move an assignment for the variable "rc" in mls_read_range_helper()
>   Move an assignment for the variable "rc" in policydb_load_isids()
>   One function call less in five functions after null pointer
> detection
>   Move two assignments for the variable "rc" in ocontext_read()
>   Return directly after a failed kzalloc() in roles_init()
>   Move two assignments for the variable "rc" in roles_init()
>   One function call less in roles_init() after error detection
>   Use kmalloc_array() in sidtab_init()
>   Adjust two checks for null pointers
>   Use common error handling code in sidtab_insert()
>   Use seq_puts() in sel_avc_stats_seq_show()
> 
>  security/selinux/selinuxfs.c      |   8 +-
>  security/selinux/ss/conditional.c |  14 +--
>  security/selinux/ss/hashtab.c     |  10 +-
>  security/selinux/ss/policydb.c    | 255 ++++++++++++++++++++------
> ------------
>  security/selinux/ss/sidtab.c      |  22 ++--
>  5 files changed, 157 insertions(+), 152 deletions(-)
>

[toc] | [prev] | [next] | [standalone]

#1559885 — Re: [PATCH 00/46] SELinux: Fine-tuning for several function implementations

FromPaul Moore <paul@paul-moore.com>
Date2017-01-16 17:50 +0100
SubjectRe: [PATCH 00/46] SELinux: Fine-tuning for several function implementations
Message-ID<t0iDL-30L-9@gated-at.bofh.it>
In reply to#1559834
On Mon, Jan 16, 2017 at 10:26 AM, Eric Paris <eparis@redhat.com> wrote:
> <opinion from irrelevant person>

Well, not totally irrelevant, I just reserve the right to ignore Eric
if I disagree with him ;)

> All of the patches look good to me except most of those which change
> the handling of `rc=`. I have a personal style preference for
>
> rc = -ENOMEM;
> val = kalloc();
> if (!val)
>   goto err;
>
> vs
>
> val = kalloc();
> if (!val) {
>   rc = -ENOMEM;
>   goto err;
> }
>
> because it saves 1 line and I think the compiler does the right/same
> thing. If there is preference among the people active in selinux
> developers (like I said, I'm now irrelevant) I guess they win.

My preference tends to be the other way around; I think putting the rc
assignment in the if block makes the code more readable and that is
how I tend to write things.  That said, I don't recall ever requiring
someone to redo a patch only because of this style nit ... if I did,
shame on me.

> But certainly a big +1 from me for the array allocation and sizeof()
> changes.

I do appreciate all the patches, thank you!  However, a bit of a
warning that it may take me a little bit of time to work my way
through reviewing and merging all of them.

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1559971 — Re: SELinux: Checking source code positions for the setting of error codes

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2017-01-16 19:40 +0100
SubjectRe: SELinux: Checking source code positions for the setting of error codes
Message-ID<t0kme-4kY-7@gated-at.bofh.it>
In reply to#1559834
> I have a personal style preference for
> 
> rc = -ENOMEM;
> val = kalloc();
> if (!val)
>   goto err;
> 
> vs
> 
> val = kalloc();
> if (!val) {
>   rc = -ENOMEM;
>   goto err;
> }
> 
> because it saves 1 line

Thanks for your feedback.


> and I think the compiler does the right/same thing.

* Did you check any corresponding code generation results in more detail?

* Can misplaced variable assignments result in unwanted run time consequences
  because of the previous approach for a control flow specification?

* How do you think about to achieve that error codes will only be set
  after a specific software failure was detected?

Regards,
Markus

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web