Re: [PATCH 10/46] selinux: Move some assignments for the variable "rc" in policydb_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:10 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 14 Jan 2017 15:22:29 +0100
>
> One local variable was set to an error code in some 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 
> ---
>  security/selinux/ss/policydb.c | 59 
> +-
>  1 file changed, 35 insertions(+), 24 deletions(-)

More code churn with no real advantage.  I agree with the style you
are using, and would support changing it if you are in the function
fixing bugs or doing other substantial changes in that code, but I
can't justify it as a standalone change, sorry.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 53e6d06e772a..506b0228d1f1 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2250,15 +2250,14 @@ int policydb_read(struct policydb *p, void *fp)
> if (rc)
> goto bad;
>
> -   rc = -EINVAL;
> if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) {
> printk(KERN_ERR "SELinux:  policydb magic number 0x%x does "
>"not match expected magic number 0x%x\n",
>le32_to_cpu(buf[0]), POLICYDB_MAGIC);
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -EINVAL;
> len = le32_to_cpu(buf[1]);
> if (len != strlen(POLICYDB_STRING)) {
> printk(KERN_ERR "SELinux:  policydb string length %d does not 
> "
> @@ -2265,11 +2265,13 @@ int policydb_read(struct policydb *p, void *fp)
>len, strlen(POLICYDB_STRING));
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -ENOMEM;
> policydb_str = kmalloc(len + 1, GFP_KERNEL);
> -   if (!policydb_str)
> +   if (!policydb_str) {
> +   rc = -ENOMEM;
> goto bad;
> +   }
>
> rc = next_entry(policydb_str, fp, len);
> if (rc) {
> @@ -2279,12 +2280,12 @@ int policydb_read(struct policydb *p, void *fp)
> goto bad;
> }
>
> -   rc = -EINVAL;
> policydb_str[len] = '\0';
> if (strcmp(policydb_str, POLICYDB_STRING)) {
> printk(KERN_ERR "SELinux:  policydb string %s does not match "
>"my string %s\n", policydb_str, POLICYDB_STRING);
> kfree(policydb_str);
> +   rc = -EINVAL;
> goto bad;
> }
> /* Done with policydb_str. */
> @@ -2296,24 +2297,24 @@ int policydb_read(struct policydb *p, void *fp)
> if (rc)
> goto bad;
>
> -   rc = -EINVAL;
> p->policyvers = le32_to_cpu(buf[0]);
> if (p->policyvers < POLICYDB_VERSION_MIN ||
> p->policyvers > POLICYDB_VERSION_MAX) {
> printk(KERN_ERR "SELinux:  policydb version %d does not match 
> "
>"my version range %d-%d\n",
>le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN, 
> POLICYDB_VERSION_MAX);
> +   rc = -EINVAL;
> goto bad;
> }
>
> if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) {
> p->mls_enabled = 1;
>
> -   rc = -EINVAL;
> if (p->policyvers < POLICYDB_VERSION_MLS) {
> printk(KERN_ERR "SELinux: security policydb version 
> %d "
> "(MLS) not backwards compatible\n",
> p->policyvers);
> +   rc = -EINVAL;
> goto bad;
> }
> }
> @@ -2332,21 +2333,21 @@ int policydb_read(struct policydb *p, void *fp)
> goto bad;
> }
>
> -   rc = -EINVAL;
> info = policydb_lookup_compat(p->policyvers);
> if (!info) {
> printk(KERN_ERR "SELinux:  unable to find policy compat info "
>"for version %d\n", p->policyvers);
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -EINVAL;
> if (le32_to_cpu(buf[2]) != info->sym_num ||
> le32_to_cpu(buf[3]) != info->ocon_num) {
> printk(KERN_ERR "SELinux:  policydb table sizes (%d,%d) do "
>"not match mine (%d,%d)\n", le32_to_cpu(buf[2]),
> le32_to_cpu(buf[3]),
>info->sym_num, info->ocon_num);
> +   rc = -EINVAL;
> goto bad;
> }
>
> @@ -2365,10 +2366,11 @@ int policydb_read(struct policydb *p, void *fp)
> p->symtab[i].nprim = nprim;
> }
>
> -   rc = -EINVAL;
> p->process_class = 

Re: [PATCH 10/46] selinux: Move some assignments for the variable "rc" in policydb_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:10 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 14 Jan 2017 15:22:29 +0100
>
> One local variable was set to an error code in some 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 
> ---
>  security/selinux/ss/policydb.c | 59 
> +-
>  1 file changed, 35 insertions(+), 24 deletions(-)

More code churn with no real advantage.  I agree with the style you
are using, and would support changing it if you are in the function
fixing bugs or doing other substantial changes in that code, but I
can't justify it as a standalone change, sorry.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 53e6d06e772a..506b0228d1f1 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2250,15 +2250,14 @@ int policydb_read(struct policydb *p, void *fp)
> if (rc)
> goto bad;
>
> -   rc = -EINVAL;
> if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) {
> printk(KERN_ERR "SELinux:  policydb magic number 0x%x does "
>"not match expected magic number 0x%x\n",
>le32_to_cpu(buf[0]), POLICYDB_MAGIC);
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -EINVAL;
> len = le32_to_cpu(buf[1]);
> if (len != strlen(POLICYDB_STRING)) {
> printk(KERN_ERR "SELinux:  policydb string length %d does not 
> "
> @@ -2265,11 +2265,13 @@ int policydb_read(struct policydb *p, void *fp)
>len, strlen(POLICYDB_STRING));
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -ENOMEM;
> policydb_str = kmalloc(len + 1, GFP_KERNEL);
> -   if (!policydb_str)
> +   if (!policydb_str) {
> +   rc = -ENOMEM;
> goto bad;
> +   }
>
> rc = next_entry(policydb_str, fp, len);
> if (rc) {
> @@ -2279,12 +2280,12 @@ int policydb_read(struct policydb *p, void *fp)
> goto bad;
> }
>
> -   rc = -EINVAL;
> policydb_str[len] = '\0';
> if (strcmp(policydb_str, POLICYDB_STRING)) {
> printk(KERN_ERR "SELinux:  policydb string %s does not match "
>"my string %s\n", policydb_str, POLICYDB_STRING);
> kfree(policydb_str);
> +   rc = -EINVAL;
> goto bad;
> }
> /* Done with policydb_str. */
> @@ -2296,24 +2297,24 @@ int policydb_read(struct policydb *p, void *fp)
> if (rc)
> goto bad;
>
> -   rc = -EINVAL;
> p->policyvers = le32_to_cpu(buf[0]);
> if (p->policyvers < POLICYDB_VERSION_MIN ||
> p->policyvers > POLICYDB_VERSION_MAX) {
> printk(KERN_ERR "SELinux:  policydb version %d does not match 
> "
>"my version range %d-%d\n",
>le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN, 
> POLICYDB_VERSION_MAX);
> +   rc = -EINVAL;
> goto bad;
> }
>
> if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) {
> p->mls_enabled = 1;
>
> -   rc = -EINVAL;
> if (p->policyvers < POLICYDB_VERSION_MLS) {
> printk(KERN_ERR "SELinux: security policydb version 
> %d "
> "(MLS) not backwards compatible\n",
> p->policyvers);
> +   rc = -EINVAL;
> goto bad;
> }
> }
> @@ -2332,21 +2333,21 @@ int policydb_read(struct policydb *p, void *fp)
> goto bad;
> }
>
> -   rc = -EINVAL;
> info = policydb_lookup_compat(p->policyvers);
> if (!info) {
> printk(KERN_ERR "SELinux:  unable to find policy compat info "
>"for version %d\n", p->policyvers);
> +   rc = -EINVAL;
> goto bad;
> }
>
> -   rc = -EINVAL;
> if (le32_to_cpu(buf[2]) != info->sym_num ||
> le32_to_cpu(buf[3]) != info->ocon_num) {
> printk(KERN_ERR "SELinux:  policydb table sizes (%d,%d) do "
>"not match mine (%d,%d)\n", le32_to_cpu(buf[2]),
> le32_to_cpu(buf[3]),
>info->sym_num, info->ocon_num);
> +   rc = -EINVAL;
> goto bad;
> }
>
> @@ -2365,10 +2366,11 @@ int policydb_read(struct policydb *p, void *fp)
> p->symtab[i].nprim = nprim;
> }
>
> -   rc = -EINVAL;
> p->process_class = string_to_security_class(p, "process");
> -   if (!p->process_class)
> +   if (!p->process_class) {
> +