Date:	Wed, 07 Aug 2013 18:44:43 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Kees Cook <keescook@...omium.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Oleg Nesterov <oleg@...hat.com>, holt@....com
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel/sys.c: improve the usage of return value


According to the related discussion in another thread, this patch can be
obsoleted now.

Thanks.

On 08/06/2013 04:43 PM, Chen Gang wrote:
> Improve the usage of return value, so not only can make code clearer
> to readers, but also can improve the performance.
> 
> Assign the return error code only when error occurs, so can save some
> instructions.
> 
> For getcpu(), uname(), newuname(), and override_release(), can remove
> the return variable to make code clearer and save some instructions.
> 
> Also modify the related code to pass "./scripts/checkpatch.pl" checking.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@...anux.com>
> ---
>  kernel/sys.c |  147 +++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 79 insertions(+), 68 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 771129b..a63a350 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -386,13 +386,14 @@ SYSCALL_DEFINE1(setgid, gid_t, gid)
>  		return -ENOMEM;
>  	old = current_cred();
>  
> -	retval = -EPERM;
>  	if (nsown_capable(CAP_SETGID))
>  		new->gid = new->egid = new->sgid = new->fsgid = kgid;
>  	else if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->sgid))
>  		new->egid = new->fsgid = kgid;
> -	else
> +	else {
> +		retval = -EPERM;
>  		goto error;
> +	}
>  
>  	return commit_creds(new);
>  
> @@ -533,7 +534,6 @@ SYSCALL_DEFINE1(setuid, uid_t, uid)
>  		return -ENOMEM;
>  	old = current_cred();
>  
> -	retval = -EPERM;
>  	if (nsown_capable(CAP_SETUID)) {
>  		new->suid = new->uid = kuid;
>  		if (!uid_eq(kuid, old->uid)) {
> @@ -542,6 +542,7 @@ SYSCALL_DEFINE1(setuid, uid_t, uid)
>  				goto error;
>  		}
>  	} else if (!uid_eq(kuid, old->uid) && !uid_eq(kuid, new->suid)) {
> +		retval = -EPERM;
>  		goto error;
>  	}
>  
> @@ -919,31 +920,35 @@ SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
>  	 */
>  	write_lock_irq(&tasklist_lock);
>  
> -	err = -ESRCH;
>  	p = find_task_by_vpid(pid);
> -	if (!p)
> +	if (!p) {
> +		err = -ESRCH;
>  		goto out;
> +	}
>  
> -	err = -EINVAL;
> -	if (!thread_group_leader(p))
> +	if (!thread_group_leader(p)) {
> +		err = -EINVAL;
>  		goto out;
> +	}
>  
>  	if (same_thread_group(p->real_parent, group_leader)) {
> -		err = -EPERM;
> -		if (task_session(p) != task_session(group_leader))
> +		if (task_session(p) != task_session(group_leader)) {
> +			err = -EPERM;
>  			goto out;
> -		err = -EACCES;
> -		if (p->did_exec)
> +		}
> +		if (p->did_exec) {
> +			err = -EACCES;
>  			goto out;
> -	} else {
> +		}
> +	} else if (p != group_leader) {
>  		err = -ESRCH;
> -		if (p != group_leader)
> -			goto out;
> +		goto out;
>  	}
>  
> -	err = -EPERM;
> -	if (p->signal->leader)
> +	if (p->signal->leader) {
> +		err = -EPERM;
>  		goto out;
> +	}
>  
>  	pgrp = task_pid(p);
>  	if (pgid != pid) {
> @@ -951,8 +956,10 @@ SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
>  
>  		pgrp = find_vpid(pgid);
>  		g = pid_task(pgrp, PIDTYPE_PGID);
> -		if (!g || task_session(g) != task_session(group_leader))
> +		if (!g || task_session(g) != task_session(group_leader)) {
> +			err = -EPERM;
>  			goto out;
> +		}
>  	}
>  
>  	err = security_task_setpgid(p, pgid);
> @@ -962,7 +969,6 @@ SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
>  	if (task_pgrp(p) != pgrp)
>  		change_pid(p, PIDTYPE_PGID, pgrp);
>  
> -	err = 0;
>  out:
>  	/* All paths lead to here, thus we are safe. -DaveM */
>  	write_unlock_irq(&tasklist_lock);
> @@ -980,14 +986,16 @@ SYSCALL_DEFINE1(getpgid, pid_t, pid)
>  	if (!pid)
>  		grp = task_pgrp(current);
>  	else {
> -		retval = -ESRCH;
>  		p = find_task_by_vpid(pid);
> -		if (!p)
> +		if (!p) {
> +			retval = -ESRCH;
>  			goto out;
> +		}
>  		grp = task_pgrp(p);
> -		if (!grp)
> +		if (!grp) {
> +			retval = -ESRCH;
>  			goto out;
> -
> +		}
>  		retval = security_task_getpgid(p);
>  		if (retval)
>  			goto out;
> @@ -1017,14 +1025,16 @@ SYSCALL_DEFINE1(getsid, pid_t, pid)
>  	if (!pid)
>  		sid = task_session(current);
>  	else {
> -		retval = -ESRCH;
>  		p = find_task_by_vpid(pid);
> -		if (!p)
> +		if (!p) {
> +			retval = -ESRCH;
>  			goto out;
> +		}
>  		sid = task_session(p);
> -		if (!sid)
> +		if (!sid) {
> +			retval = -ESRCH;
>  			goto out;
> -
> +		}
>  		retval = security_task_getsid(p);
>  		if (retval)
>  			goto out;
> @@ -1096,8 +1106,6 @@ DECLARE_RWSEM(uts_sem);
>   */
>  static int override_release(char __user *release, size_t len)
>  {
> -	int ret = 0;
> -
>  	if (current->personality & UNAME26) {
>  		const char *rest = UTS_RELEASE;
>  		char buf[65] = { 0 };
> @@ -1115,25 +1123,25 @@ static int override_release(char __user *release, size_t len)
>  		v = ((LINUX_VERSION_CODE >> 8) & 0xff) + 40;
>  		copy = clamp_t(size_t, len, 1, sizeof(buf));
>  		copy = scnprintf(buf, copy, "2.6.%u%s", v, rest);
> -		ret = copy_to_user(release, buf, copy + 1);
> +		return copy_to_user(release, buf, copy + 1);
>  	}
> -	return ret;
> +	return 0;
>  }
>  
>  SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
>  {
> -	int errno = 0;
> -
>  	down_read(&uts_sem);
> -	if (copy_to_user(name, utsname(), sizeof *name))
> -		errno = -EFAULT;
> +	if (copy_to_user(name, utsname(), sizeof(*name))) {
> +		up_read(&uts_sem);
> +		return -EFAULT;
> +	}
>  	up_read(&uts_sem);
>  
> -	if (!errno && override_release(name->release, sizeof(name->release)))
> -		errno = -EFAULT;
> -	if (!errno && override_architecture(name))
> -		errno = -EFAULT;
> -	return errno;
> +	if (override_release(name->release, sizeof(name->release)))
> +		return -EFAULT;
> +	if (override_architecture(name))
> +		return -EFAULT;
> +	return 0;
>  }
>  
>  #ifdef __ARCH_WANT_SYS_OLD_UNAME
> @@ -1142,21 +1150,21 @@ SYSCALL_DEFINE1(newuname, struct new_utsname __user *, name)
>   */
>  SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
>  {
> -	int error = 0;
> -
>  	if (!name)
>  		return -EFAULT;
>  
>  	down_read(&uts_sem);
> -	if (copy_to_user(name, utsname(), sizeof(*name)))
> -		error = -EFAULT;
> +	if (copy_to_user(name, utsname(), sizeof(*name))) {
> +		up_read(&uts_sem);
> +		return -EFAULT;
> +	}
>  	up_read(&uts_sem);
>  
> -	if (!error && override_release(name->release, sizeof(name->release)))
> -		error = -EFAULT;
> -	if (!error && override_architecture(name))
> -		error = -EFAULT;
> -	return error;
> +	if (override_release(name->release, sizeof(name->release)))
> +		return -EFAULT;
> +	if (override_architecture(name))
> +		return -EFAULT;
> +	return 0;
>  }
>  
>  SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
> @@ -1185,12 +1193,14 @@ SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
>  				__OLD_UTS_LEN);
>  	error |= __put_user(0, name->machine + __OLD_UTS_LEN);
>  	up_read(&uts_sem);
> +	if (error)
> +		return -EFAULT;
>  
> -	if (!error && override_architecture(name))
> -		error = -EFAULT;
> -	if (!error && override_release(name->release, sizeof(name->release)))
> -		error = -EFAULT;
> -	return error ? -EFAULT : 0;
> +	if (override_architecture(name))
> +		return -EFAULT;
> +	if (override_release(name->release, sizeof(name->release)))
> +		return -EFAULT;
> +	return 0;
>  }
>  #endif
>  
> @@ -1648,10 +1658,11 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  	 * sure that this one is executable as well, to avoid breaking an
>  	 * overall picture.
>  	 */
> -	err = -EACCES;
> -	if (!S_ISREG(inode->i_mode)	||
> -	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (!S_ISREG(inode->i_mode) ||
> +	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
> +		err = -EACCES;
>  		goto exit;
> +	}
>  
>  	err = inode_permission(inode, MAY_EXEC);
>  	if (err)
> @@ -1662,15 +1673,16 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  	/*
>  	 * Forbid mm->exe_file change if old file still mapped.
>  	 */
> -	err = -EBUSY;
>  	if (mm->exe_file) {
>  		struct vm_area_struct *vma;
>  
>  		for (vma = mm->mmap; vma; vma = vma->vm_next)
>  			if (vma->vm_file &&
>  			    path_equal(&vma->vm_file->f_path,
> -				       &mm->exe_file->f_path))
> +				       &mm->exe_file->f_path)) {
> +				err = -EBUSY;
>  				goto exit_unlock;
> +			}
>  	}
>  
>  	/*
> @@ -1679,11 +1691,10 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  	 * could make a snapshot over all processes running and monitor
>  	 * /proc/pid/exe changes to notice unusual activity if needed.
>  	 */
> -	err = -EPERM;
> -	if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
> +	if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags)) {
> +		err = -EPERM;
>  		goto exit_unlock;
> -
> -	err = 0;
> +	}
>  	set_mm_exe_file(mm, exe.file);	/* this grabs a reference to exe.file */
>  exit_unlock:
>  	up_write(&mm->mmap_sem);
> @@ -2009,13 +2020,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  SYSCALL_DEFINE3(getcpu, unsigned __user *, cpup, unsigned __user *, nodep,
>  		struct getcpu_cache __user *, unused)
>  {
> -	int err = 0;
>  	int cpu = raw_smp_processor_id();
> -	if (cpup)
> -		err |= put_user(cpu, cpup);
> -	if (nodep)
> -		err |= put_user(cpu_to_node(cpu), nodep);
> -	return err ? -EFAULT : 0;
> +
> +	if (cpup && put_user(cpu, cpup))
> +		return -EFAULT;
> +	if (nodep && put_user(cpu_to_node(cpu), nodep))
> +		return -EFAULT;
> +	return 0;
>  }
>  
>  /**
> 


-- 
Chen Gang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists