[09/46] selinux: Delete an error message for a failed memory allocation in policydb_read()

Commit Message

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

From: Markus Elfring <[e-mail]>
Date: Sat, 14 Jan 2017 14:20:41 +0100

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <[e-mail]>
---
 security/selinux/ss/policydb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Paul Moore March 23, 2017, 9:33 p.m. UTC | #1

On Sun, Jan 15, 2017 at 10:07 AM, SF Markus Elfring
<[e-mail]> wrote:
> From: Markus Elfring <[e-mail]>
> Date: Sat, 14 Jan 2017 14:20:41 +0100
>
> Omit an extra message for a memory allocation failure in this function.
>
> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>  security/selinux/ss/policydb.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

I'm not going to remove an error message without some better reasoning
in the patch description.  Providing a link to slides is fine, but
your commit message needs to convey the important information and I
don't think that is the case here (what happens when that URL dies?).

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index fe8992382a71..53e6d06e772a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2269,11 +2269,8 @@ int policydb_read(struct policydb *p, void *fp)
>
>         rc = -ENOMEM;
>         policydb_str = kmalloc(len + 1, GFP_KERNEL);
> -       if (!policydb_str) {
> -               printk(KERN_ERR "SELinux:  unable to allocate memory for policydb "
> -                      "string of length %d\n", len);
> +       if (!policydb_str)
>                 goto bad;
> -       }
>
>         rc = next_entry(policydb_str, fp, len);
>         if (rc) {
> --
> 2.11.0
>

SF Markus Elfring March 24, 2017, 12:13 p.m. UTC | #2

>> Omit an extra message for a memory allocation failure in this function.
>>
>> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
>> Signed-off-by: Markus Elfring <[e-mail]>
>> ---
>>  security/selinux/ss/policydb.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> I'm not going to remove an error message without some better reasoning
> in the patch description.  Providing a link to slides is fine, but
> your commit message needs to convey the important information and I
> don't think that is the case here (what happens when that URL dies?).

Do you need an explicit reminder there that the function “kmalloc” provides its own
error reporting already because the flag “__GFP_NOWARN” was not passed here?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore March 25, 2017, 3:44 p.m. UTC | #3

On Fri, Mar 24, 2017 at 8:13 AM, SF Markus Elfring
<[e-mail]> wrote:
>>> Omit an extra message for a memory allocation failure in this function.
>>>
>>> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
>>> Signed-off-by: Markus Elfring <[e-mail]>
>>> ---
>>>  security/selinux/ss/policydb.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> I'm not going to remove an error message without some better reasoning
>> in the patch description.  Providing a link to slides is fine, but
>> your commit message needs to convey the important information and I
>> don't think that is the case here (what happens when that URL dies?).
>
> Do you need an explicit reminder there that the function “kmalloc” provides its own
> error reporting already because the flag “__GFP_NOWARN” was not passed here?

That is what I said by "better reasoning in the patch description",
however, now that I'm looking at this again, I don't think I'm going
to merge this.  Yes, maybe in some cases it is a bit wasteful, but I
like the error message.

SF Markus Elfring March 27, 2017, 5:56 a.m. UTC | #4

> …, but I like the error message.

How do you think about to pass the flag “__GFP_NOWARN” if you like
this information more than the default error reporting of the function “kmalloc”?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore March 27, 2017, 6:23 p.m. UTC | #5

On Mon, Mar 27, 2017 at 1:56 AM, SF Markus Elfring
<[e-mail]> wrote:
>> …, but I like the error message.
>
> How do you think about to pass the flag “__GFP_NOWARN” if you like
> this information more than the default error reporting of the function “kmalloc”?

Possibly, although I would encourage you to just leave it as-is for
the moment.  Reviewing and merging patches carries a cost, and I would
very much prefer to allocate my time/resources on changes that have a
more significant impact.  I don't want to discourage your from
contributing to SELinux development, but I do want to strongly
encourage you to contribute more meaningful patches.

Patch

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index fe8992382a71..53e6d06e772a 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2269,11 +2269,8 @@  int policydb_read(struct policydb *p, void *fp)
 
 	rc = -ENOMEM;
 	policydb_str = kmalloc(len + 1, GFP_KERNEL);
-	if (!policydb_str) {
-		printk(KERN_ERR "SELinux:  unable to allocate memory for policydb "
-		       "string of length %d\n", len);
+	if (!policydb_str)
 		goto bad;
-	}
 
 	rc = next_entry(policydb_str, fp, len);
 	if (rc) {