[07/46] selinux: Delete unnecessary variable assignments in policydb_index()

Commit Message

SF Markus Elfring Jan. 15, 2017, 3:04 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Sat, 14 Jan 2017 13:40:25 +0100

The local variable "rc" was reset with an error code up to five times
before a memory allocation failure was detected.

Add a jump target so that this assignment will only be performed after
a concrete software failure.

Signed-off-by: Markus Elfring <[e-mail]>
---
 security/selinux/ss/policydb.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Casey Schaufler Jan. 17, 2017, 4:27 p.m. UTC | #1

On 1/15/2017 7:04 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> Date: Sat, 14 Jan 2017 13:40:25 +0100
>
> The local variable "rc" was reset with an error code up to five times
> before a memory allocation failure was detected.
>
> Add a jump target so that this assignment will only be performed after
> a concrete software failure.
>
> Signed-off-by: Markus Elfring <[e-mail]>

Of course the maintainers get the call on this,
but I see this as an example of the kind of code
that led to the "gotos considered harmful" mindset.
What value does it add? All I see is unnecessary
code churn. And a backwards goto.


> ---
>  security/selinux/ss/policydb.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 21869b622c0c..4d4ba1ad910d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -539,34 +539,30 @@ static int policydb_index(struct policydb *p)
>  	symtab_hash_eval(p->symtab);
>  #endif
>  
> -	rc = -ENOMEM;
>  	p->class_val_to_struct = kcalloc(p->p_classes.nprim,
>  					 sizeof(*p->class_val_to_struct),
>  					 GFP_KERNEL);
>  	if (!p->class_val_to_struct)
> -		goto out;
> +		goto failure_indication;
>  
> -	rc = -ENOMEM;
>  	p->role_val_to_struct = kcalloc(p->p_roles.nprim,
>  					sizeof(*p->role_val_to_struct),
>  					GFP_KERNEL);
>  	if (!p->role_val_to_struct)
> -		goto out;
> +		goto failure_indication;
>  
> -	rc = -ENOMEM;
>  	p->user_val_to_struct = kcalloc(p->p_users.nprim,
>  					sizeof(*p->user_val_to_struct),
>  					GFP_KERNEL);
>  	if (!p->user_val_to_struct)
> -		goto out;
> +		goto failure_indication;
>  
>  	/* Yes, I want the sizeof the pointer, not the structure */
> -	rc = -ENOMEM;
>  	p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
>  						       p->p_types.nprim,
>  						       GFP_KERNEL | __GFP_ZERO);
>  	if (!p->type_val_to_struct_array)
> -		goto out;
> +		goto failure_indication;
>  
>  	rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
>  				 p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
> @@ -578,12 +574,11 @@ static int policydb_index(struct policydb *p)
>  		goto out;
>  
>  	for (i = 0; i < SYM_NUM; i++) {
> -		rc = -ENOMEM;
>  		p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
>  							 p->symtab[i].nprim,
>  							 GFP_KERNEL | __GFP_ZERO);
>  		if (!p->sym_val_to_name[i])
> -			goto out;
> +			goto failure_indication;
>  
>  		rc = flex_array_prealloc(p->sym_val_to_name[i],
>  					 0, p->symtab[i].nprim,
> @@ -598,6 +593,9 @@ static int policydb_index(struct policydb *p)
>  	rc = 0;
>  out:
>  	return rc;
> +failure_indication:
> +	rc = -ENOMEM;
> +	goto out;
>  }
>  
>  /*

Paul Moore March 23, 2017, 9:20 p.m. UTC | #2

On Sun, Jan 15, 2017 at 10:04 AM, SF Markus Elfring
<[e-mail]> wrote:
> From: Markus Elfring <[e-mail]>
> Date: Sat, 14 Jan 2017 13:40:25 +0100
>
> The local variable "rc" was reset with an error code up to five times
> before a memory allocation failure was detected.
>
> Add a jump target so that this assignment will only be performed after
> a concrete software failure.
>
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>  security/selinux/ss/policydb.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

Like you, I generally despise the notion of unconditionally setting
return codes to an error value *before* the faulting call; see Eric's
response on patch 0/46, my preferred style is different from Eric.
However, I agree with Casey that this patch is mostly just code churn
so I'm going to drop this from your series.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 21869b622c0c..4d4ba1ad910d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -539,34 +539,30 @@ static int policydb_index(struct policydb *p)
>         symtab_hash_eval(p->symtab);
>  #endif
>
> -       rc = -ENOMEM;
>         p->class_val_to_struct = kcalloc(p->p_classes.nprim,
>                                          sizeof(*p->class_val_to_struct),
>                                          GFP_KERNEL);
>         if (!p->class_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
> -       rc = -ENOMEM;
>         p->role_val_to_struct = kcalloc(p->p_roles.nprim,
>                                         sizeof(*p->role_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->role_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
> -       rc = -ENOMEM;
>         p->user_val_to_struct = kcalloc(p->p_users.nprim,
>                                         sizeof(*p->user_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->user_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
>         /* Yes, I want the sizeof the pointer, not the structure */
> -       rc = -ENOMEM;
>         p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
>                                                        p->p_types.nprim,
>                                                        GFP_KERNEL | __GFP_ZERO);
>         if (!p->type_val_to_struct_array)
> -               goto out;
> +               goto failure_indication;
>
>         rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
>                                  p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
> @@ -578,12 +574,11 @@ static int policydb_index(struct policydb *p)
>                 goto out;
>
>         for (i = 0; i < SYM_NUM; i++) {
> -               rc = -ENOMEM;
>                 p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
>                                                          p->symtab[i].nprim,
>                                                          GFP_KERNEL | __GFP_ZERO);
>                 if (!p->sym_val_to_name[i])
> -                       goto out;
> +                       goto failure_indication;
>
>                 rc = flex_array_prealloc(p->sym_val_to_name[i],
>                                          0, p->symtab[i].nprim,
> @@ -598,6 +593,9 @@ static int policydb_index(struct policydb *p)
>         rc = 0;
>  out:
>         return rc;
> +failure_indication:
> +       rc = -ENOMEM;
> +       goto out;
>  }
>
>  /*
> --
> 2.11.0
>

SF Markus Elfring March 27, 2017, 6:24 a.m. UTC | #3

> However, I agree with Casey that this patch is mostly just code churn
> so I'm going to drop this from your series.

How do you think about to return only constant error codes in this function?
Would it be acceptable to replace any statements “goto out;” with
“return -ENOMEM;” here instead?

Regards,
Markus

Paul Moore March 27, 2017, 6:20 p.m. UTC | #4

On Mon, Mar 27, 2017 at 2:24 AM, SF Markus Elfring
<[e-mail]> wrote:
>> However, I agree with Casey that this patch is mostly just code churn
>> so I'm going to drop this from your series.
>
> How do you think about to return only constant error codes in this function?
> Would it be acceptable to replace any statements “goto out;” with
> “return -ENOMEM;” here instead?

Yes.

Patch

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 21869b622c0c..4d4ba1ad910d 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -539,34 +539,30 @@  static int policydb_index(struct policydb *p)
 	symtab_hash_eval(p->symtab);
 #endif
 
-	rc = -ENOMEM;
 	p->class_val_to_struct = kcalloc(p->p_classes.nprim,
 					 sizeof(*p->class_val_to_struct),
 					 GFP_KERNEL);
 	if (!p->class_val_to_struct)
-		goto out;
+		goto failure_indication;
 
-	rc = -ENOMEM;
 	p->role_val_to_struct = kcalloc(p->p_roles.nprim,
 					sizeof(*p->role_val_to_struct),
 					GFP_KERNEL);
 	if (!p->role_val_to_struct)
-		goto out;
+		goto failure_indication;
 
-	rc = -ENOMEM;
 	p->user_val_to_struct = kcalloc(p->p_users.nprim,
 					sizeof(*p->user_val_to_struct),
 					GFP_KERNEL);
 	if (!p->user_val_to_struct)
-		goto out;
+		goto failure_indication;
 
 	/* Yes, I want the sizeof the pointer, not the structure */
-	rc = -ENOMEM;
 	p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
 						       p->p_types.nprim,
 						       GFP_KERNEL | __GFP_ZERO);
 	if (!p->type_val_to_struct_array)
-		goto out;
+		goto failure_indication;
 
 	rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
 				 p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
@@ -578,12 +574,11 @@  static int policydb_index(struct policydb *p)
 		goto out;
 
 	for (i = 0; i < SYM_NUM; i++) {
-		rc = -ENOMEM;
 		p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
 							 p->symtab[i].nprim,
 							 GFP_KERNEL | __GFP_ZERO);
 		if (!p->sym_val_to_name[i])
-			goto out;
+			goto failure_indication;
 
 		rc = flex_array_prealloc(p->sym_val_to_name[i],
 					 0, p->symtab[i].nprim,
@@ -598,6 +593,9 @@  static int policydb_index(struct policydb *p)
 	rc = 0;
 out:
 	return rc;
+failure_indication:
+	rc = -ENOMEM;
+	goto out;
 }
 
 /*