[PATCH 0/2] selinux: Clean up GFP flag usage

[PATCH 0/2] selinux: Clean up GFP flag usage

[Ondrej Mosnacek]

This series corrects a few places where GFP_ATOMIC was used instead of
GFP_KERNEL for no reason and removes the no longer needed GFP flag
argument from str_read().

Ondrej Mosnacek (2):
  selinux: switch unnecessary GFP_ATOMIC allocs to GFP_KERNEL
  selinux: drop the gfp_t argument from str_read()

 security/selinux/hooks.c       |  6 ++---
 security/selinux/ss/policydb.c | 42 +++++++++++++++++-----------------
 2 files changed, 24 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH 1/2] selinux: switch unnecessary GFP_ATOMIC allocs to GFP_KERNEL

[Ondrej Mosnacek]

There seems to be no reason to use GFP_ATOMIC in these cases.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c       |  6 +++---
 security/selinux/ss/policydb.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 89d3753b7bd5d..4de962daffbde 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3171,7 +3171,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 				audit_size = 0;
 			}
 			ab = audit_log_start(audit_context(),
-					     GFP_ATOMIC, AUDIT_SELINUX_ERR);
+					     GFP_KERNEL, AUDIT_SELINUX_ERR);
 			audit_log_format(ab, "op=setxattr invalid_context=");
 			audit_log_n_untrustedstring(ab, value, audit_size);
 			audit_log_end(ab);
@@ -6388,7 +6388,7 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
 				else
 					audit_size = size;
 				ab = audit_log_start(audit_context(),
-						     GFP_ATOMIC,
+						     GFP_KERNEL,
 						     AUDIT_SELINUX_ERR);
 				audit_log_format(ab, "op=fscreate invalid_context=");
 				audit_log_n_untrustedstring(ab, value, audit_size);
@@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what)
 
 	if (WARN(invalid_reason, "Invalid lockdown reason")) {
 		audit_log(audit_context(),
-			  GFP_ATOMIC, AUDIT_SELINUX_ERR,
+			  GFP_KERNEL, AUDIT_SELINUX_ERR,
 			  "lockdown_reason=invalid");
 		return -EINVAL;
 	}
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 9fccf417006b0..c1437de04e1d9 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1577,7 +1577,7 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp)
 	__le32 buf[2];
 	u32 len;
 
-	levdatum = kzalloc(sizeof(*levdatum), GFP_ATOMIC);
+	levdatum = kzalloc(sizeof(*levdatum), GFP_KERNEL);
 	if (!levdatum)
 		return -ENOMEM;
 
@@ -1588,12 +1588,12 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp)
 	len = le32_to_cpu(buf[0]);
 	levdatum->isalias = le32_to_cpu(buf[1]);
 
-	rc = str_read(&key, GFP_ATOMIC, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
 	rc = -ENOMEM;
-	levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_ATOMIC);
+	levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_KERNEL);
 	if (!levdatum->level)
 		goto bad;
 
@@ -1618,7 +1618,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp)
 	__le32 buf[3];
 	u32 len;
 
-	catdatum = kzalloc(sizeof(*catdatum), GFP_ATOMIC);
+	catdatum = kzalloc(sizeof(*catdatum), GFP_KERNEL);
 	if (!catdatum)
 		return -ENOMEM;
 
@@ -1630,7 +1630,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp)
 	catdatum->value = le32_to_cpu(buf[1]);
 	catdatum->isalias = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_ATOMIC, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
-- 
2.26.2

[PATCH 2/2] selinux: drop the gfp_t argument from str_read()

[Ondrej Mosnacek]

Now that all callers specify GFP_KERNEL here, the argument is no longer
necessary and the function can just use GFP_KERNEL directly.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/policydb.c | 36 +++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index c1437de04e1d9..a7b6b2f5d71d9 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1074,7 +1074,7 @@ out:
  * binary representation file.
  */
 
-static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
+static int str_read(char **strp, void *fp, u32 len)
 {
 	int rc;
 	char *str;
@@ -1082,7 +1082,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
 	if ((len == 0) || (len == (u32)-1))
 		return -EINVAL;
 
-	str = kmalloc(len + 1, flags | __GFP_NOWARN);
+	str = kmalloc(len + 1, GFP_KERNEL | __GFP_NOWARN);
 	if (!str)
 		return -ENOMEM;
 
@@ -1116,7 +1116,7 @@ static int perm_read(struct policydb *p, struct symtab *s, void *fp)
 	len = le32_to_cpu(buf[0]);
 	perdatum->value = le32_to_cpu(buf[1]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1155,7 +1155,7 @@ static int common_read(struct policydb *p, struct symtab *s, void *fp)
 		goto bad;
 	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1322,12 +1322,12 @@ static int class_read(struct policydb *p, struct symtab *s, void *fp)
 
 	ncons = le32_to_cpu(buf[5]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
 	if (len2) {
-		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
+		rc = str_read(&cladatum->comkey, fp, len2);
 		if (rc)
 			goto bad;
 
@@ -1413,7 +1413,7 @@ static int role_read(struct policydb *p, struct symtab *s, void *fp)
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		role->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1479,7 +1479,7 @@ static int type_read(struct policydb *p, struct symtab *s, void *fp)
 		typdatum->primary = le32_to_cpu(buf[2]);
 	}
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1543,7 +1543,7 @@ static int user_read(struct policydb *p, struct symtab *s, void *fp)
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		usrdatum->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1588,7 +1588,7 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp)
 	len = le32_to_cpu(buf[0]);
 	levdatum->isalias = le32_to_cpu(buf[1]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1630,7 +1630,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp)
 	catdatum->value = le32_to_cpu(buf[1]);
 	catdatum->isalias = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1904,7 +1904,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, fp, len);
 	if (rc)
 		return rc;
 
@@ -1988,7 +1988,7 @@ static int filename_trans_read_helper(struct policydb *p, void *fp)
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, fp, len);
 	if (rc)
 		return rc;
 
@@ -2128,7 +2128,7 @@ static int genfs_read(struct policydb *p, void *fp)
 		if (!newgenfs)
 			goto out;
 
-		rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
+		rc = str_read(&newgenfs->fstype, fp, len);
 		if (rc)
 			goto out;
 
@@ -2167,7 +2167,7 @@ static int genfs_read(struct policydb *p, void *fp)
 			if (!newc)
 				goto out;
 
-			rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
+			rc = str_read(&newc->u.name, fp, len);
 			if (rc)
 				goto out;
 
@@ -2261,7 +2261,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 					goto out;
 				len = le32_to_cpu(buf[0]);
 
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, fp, len);
 				if (rc)
 					goto out;
 
@@ -2307,7 +2307,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 					goto out;
 
 				len = le32_to_cpu(buf[1]);
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, fp, len);
 				if (rc)
 					goto out;
 
@@ -2370,7 +2370,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 					goto out;
 				len = le32_to_cpu(buf[0]);
 
-				rc = str_read(&c->u.ibendport.dev_name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.ibendport.dev_name, fp, len);
 				if (rc)
 					goto out;
 
-- 
2.26.2

Re: [PATCH 1/2] selinux: switch unnecessary GFP_ATOMIC allocs to GFP_KERNEL

[Ondrej Mosnacek]

On Mon, Aug 24, 2020 at 5:52 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> There seems to be no reason to use GFP_ATOMIC in these cases.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/hooks.c       |  6 +++---
>  security/selinux/ss/policydb.c | 10 +++++-----
>  2 files changed, 8 insertions(+), 8 deletions(-)

I found at least one more unjustified GFP_ATOMIC in services.c, so
I'll probably respin this patch so it is all in one commit. I didn't
bother looking at services.c at first, since most of the allocations
there are bound to GFP_ATOMIC due to the policy read lock being held.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.