[PATCH 0/4] alpha: Fine-tuning for five function implementations

[PATCH 0/4] alpha: Fine-tuning for five function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 12:26:38 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Return an error code only as a constant in osf_sigstack()
  Move two assignments for the variable "error" in osf_utsname()
  Return directly after a failed copy_from_user() or getname() in two functions
  Move two assignments for the variable "res" in srm_env_proc_write()

 arch/alpha/kernel/osf_sys.c | 51 ++++++++++++++-------------------------------
 arch/alpha/kernel/srm_env.c | 10 +++++----
 2 files changed, 22 insertions(+), 39 deletions(-)

-- 
2.11.0

[PATCH 1/4] alpha: Return an error code only as a constant in osf_sigstack()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 10:40:51 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the local variable "error" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/alpha/kernel/osf_sys.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 56e427c7aa3c..41174156a676 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -691,20 +691,17 @@ SYSCALL_DEFINE2(osf_sigstack, struct sigstack __user *, uss,
 	unsigned long usp = rdusp();
 	unsigned long oss_sp = current->sas_ss_sp + current->sas_ss_size;
 	unsigned long oss_os = on_sig_stack(usp);
-	int error;
 
 	if (uss) {
 		void __user *ss_sp;
 
-		error = -EFAULT;
 		if (get_user(ss_sp, &uss->ss_sp))
-			goto out;
+			return -EFAULT;
 
 		/* If the current stack was set with sigaltstack, don't
 		   swap stacks while we are on it.  */
-		error = -EPERM;
 		if (current->sas_ss_sp && on_sig_stack(usp))
-			goto out;
+			return -EPERM;
 
 		/* Since we don't know the extent of the stack, and we don't
 		   track onstack-ness, but rather calculate it, we must 
@@ -714,16 +711,12 @@ SYSCALL_DEFINE2(osf_sigstack, struct sigstack __user *, uss,
 	}
 
 	if (uoss) {
-		error = -EFAULT;
 		if (! access_ok(VERIFY_WRITE, uoss, sizeof(*uoss))
 		    || __put_user(oss_sp, &uoss->ss_sp)
 		    || __put_user(oss_os, &uoss->ss_onstack))
-			goto out;
+			return -EFAULT;
 	}
-
-	error = 0;
- out:
-	return error;
+	return 0;
 }
 
 SYSCALL_DEFINE3(osf_sysinfo, int, command, char __user *, buf, long, count)
-- 
2.11.0

[PATCH 2/4] alpha: Move two assignments for the variable "error" in osf_utsname()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 11:10:03 +0100

A local variable was set to an error code in one case before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/alpha/kernel/osf_sys.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 41174156a676..73ff5d698591 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -528,20 +528,14 @@ SYSCALL_DEFINE1(osf_utsname, char __user *, name)
 	int error;
 
 	down_read(&uts_sem);
-	error = -EFAULT;
-	if (copy_to_user(name + 0, utsname()->sysname, 32))
-		goto out;
-	if (copy_to_user(name + 32, utsname()->nodename, 32))
-		goto out;
-	if (copy_to_user(name + 64, utsname()->release, 32))
-		goto out;
-	if (copy_to_user(name + 96, utsname()->version, 32))
-		goto out;
-	if (copy_to_user(name + 128, utsname()->machine, 32))
-		goto out;
-
-	error = 0;
- out:
+	if (copy_to_user(name + 0, utsname()->sysname, 32) ||
+	    copy_to_user(name + 32, utsname()->nodename, 32) ||
+	    copy_to_user(name + 64, utsname()->release, 32) ||
+	    copy_to_user(name + 96, utsname()->version, 32) ||
+	    copy_to_user(name + 128, utsname()->machine, 32))
+		error = -EFAULT;
+	else
+		error = 0;
 	up_read(&uts_sem);	
 	return error;
 }
-- 
2.11.0

[PATCH 3/4] alpha: Return directly after a failed copy_from_user() or getname() in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 11:30:06 +0100

Return directly after a call of the function "copy_from_user" or "getname"
failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/alpha/kernel/osf_sys.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 73ff5d698591..4310bc79d09c 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -454,16 +454,13 @@ osf_ufs_mount(const char __user *dirname,
 	struct cdfs_args tmp;
 	struct filename *devname;
 
-	retval = -EFAULT;
 	if (copy_from_user(&tmp, args, sizeof(tmp)))
-		goto out;
+		return -EFAULT;
 	devname = getname(tmp.devname);
-	retval = PTR_ERR(devname);
 	if (IS_ERR(devname))
-		goto out;
+		return PTR_ERR(devname);
 	retval = do_mount(devname->name, dirname, "ext2", flags, NULL);
 	putname(devname);
- out:
 	return retval;
 }
 
@@ -475,16 +472,13 @@ osf_cdfs_mount(const char __user *dirname,
 	struct cdfs_args tmp;
 	struct filename *devname;
 
-	retval = -EFAULT;
 	if (copy_from_user(&tmp, args, sizeof(tmp)))
-		goto out;
+		return -EFAULT;
 	devname = getname(tmp.devname);
-	retval = PTR_ERR(devname);
 	if (IS_ERR(devname))
-		goto out;
+		return PTR_ERR(devname);
 	retval = do_mount(devname->name, dirname, "iso9660", flags, NULL);
 	putname(devname);
- out:
 	return retval;
 }
 
-- 
2.11.0

[PATCH 4/4] alpha: Move two assignments for the variable "res" in srm_env_proc_write()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 12:08:44 +0100

A local variable was set to an error code in two cases before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/alpha/kernel/srm_env.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/kernel/srm_env.c b/arch/alpha/kernel/srm_env.c
index ffe996a54fad..a5f182780578 100644
--- a/arch/alpha/kernel/srm_env.c
+++ b/arch/alpha/kernel/srm_env.c
@@ -113,13 +113,15 @@ static ssize_t srm_env_proc_write(struct file *file, const char __user *buffer,
 	if (!buf)
 		return -ENOMEM;
 
-	res = -EINVAL;
-	if (count >= PAGE_SIZE)
+	if (count >= PAGE_SIZE) {
+		res = -EINVAL;
 		goto out;
+	}
 
-	res = -EFAULT;
-	if (copy_from_user(buf, buffer, count))
+	if (copy_from_user(buf, buffer, count)) {
+		res = -EFAULT;
 		goto out;
+	}
 	buf[count] = '\0';
 
 	ret1 = callback_setenv(id, buf, count);
-- 
2.11.0

Re: [PATCH 4/4] alpha: Move two assignments for the variable "res" in srm_env_proc_write()

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On Wed, 2017-01-18 12:50:17 +0100, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Jan 2017 12:08:44 +0100
> 
> A local variable was set to an error code in two cases before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Acked-by: Jan-Benedict Glaw <jbglaw@lug-owl.de>

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:            http://www.chiark.greenend.org.uk/~sgtatham/bugs.html
the second  :

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/4] alpha: Move two assignments for the variable "res" in srm_env_proc_write()

[SF Markus Elfring]

>> A local variable was set to an error code in two cases before a concrete
>> error situation was detected. Thus move the corresponding assignment into
>> an if branch to indicate a software failure there.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> Acked-by: Jan-Benedict Glaw <jbglaw@lug-owl.de>

Thanks for your positive feedback.


Unfortunately, I have got the impression that this update step is inappropriate
so far because the suggested change for the handling of the error code “-EFAULT”
can be incomplete (or a bit too much).

Which implementation would you prefer?

A) Keep the variable assignment before the check for the call of
   the function “copy_from_user”

B) Add an assignment in another condition branch at the end.

 		res = (int) ret1;
+	} else {
+		res = -EFAULT;
 	}

Regards,
Markus

Re: [PATCH 4/4] alpha: Move two assignments for the variable "res" in srm_env_proc_write()

[Al Viro]

On Wed, Jan 18, 2017 at 12:50:17PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Jan 2017 12:08:44 +0100
> 
> A local variable was set to an error code in two cases before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.

Why the hell is that an issue?  It's a common enough idiom, and while these
functions are far from being hot paths, blind patches like that are very
much to be discouraged.  NAK.

Re: alpha: Checking source code positions for the setting of error codes

[SF Markus Elfring]

>> A local variable was set to an error code in two cases before a concrete
>> error situation was detected. Thus move the corresponding assignment into
>> an if branch to indicate a software failure there.
>>
>> This issue was detected by using the Coccinelle software.
> 
> Why the hell is that an issue?

* Can misplaced variable assignments result in unwanted run time consequences
  because of the previous approach for a control flow specification?

* How do you think about to achieve that error codes will only be set
  after a specific software failure was detected?


> It's a common enough idiom,

Are corresponding implementation details worth for another look?


> and while these functions are far from being hot paths,
> blind patches like that are very much to be discouraged.  NAK.

Thanks for your feedback.

Regards,
Markus

Re: alpha: Checking source code positions for the setting of error codes

[Al Viro]

On Wed, Jan 18, 2017 at 04:41:10PM +0100, SF Markus Elfring wrote:
> >> A local variable was set to an error code in two cases before a concrete
> >> error situation was detected. Thus move the corresponding assignment into
> >> an if branch to indicate a software failure there.
> >>
> >> This issue was detected by using the Coccinelle software.
> > 
> > Why the hell is that an issue?
> 
> * Can misplaced variable assignments result in unwanted run time consequences
>   because of the previous approach for a control flow specification?

More like the opposite.
	load constant to register
	test
	branch usually not taken
is considerably cheaper than
	test
	branch usually taken

Something like
	if (unlikely(foo)) {
		err = -ESOMETHING;
		goto sod_off;
	}
would be more or less on par (and quite possibly would be compiled into
the same code - depends upon the scheduling details for processor,
but speculative load of constant can be an optimization).  However, that
has an effect of splattering the source with tons of those unlikely() *and*
visually cluttering the common path.

> * How do you think about to achieve that error codes will only be set
>   after a specific software failure was detected?

Sounds like an arbitrary requirement, TBH...

Again, loading a constant into register tends to be cheap and easy to
combine with other instructions at CPU pipeline level.  If anything, this
pattern is a microoptimization, often in spots that are not on hotpaths
by any stretch of imagination.  But estimating whether a given place is
on a hot path takes a lot more delicate analysis than feasible for
cocci scripts.  And visual cluttering of the common execution path remains -
it doesn't matter for compiler, but it can matter a lot for human readers.

Re: [PATCH 4/4] alpha: Move two assignments for the variable "res" in srm_env_proc_write()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

Hi Markus,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.10-rc4 next-20170118]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/alpha-Fine-tuning-for-five-function-implementations/20170119-075247
config: alpha-defconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   arch/alpha/kernel/srm_env.c: In function 'srm_env_proc_write':
>> arch/alpha/kernel/srm_env.c:108:6: warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized]
     int res;
         ^~~

vim +/res +108 arch/alpha/kernel/srm_env.c

0ead0f84 Alexey Dobriyan   2009-12-14   92  		seq_write(m, page, ret);
0ead0f84 Alexey Dobriyan   2009-12-14   93  		ret = 0;
16b7f4dc Jan-Benedict Glaw 2006-11-07   94  	} else
0ead0f84 Alexey Dobriyan   2009-12-14   95  		ret = -EFAULT;
0ead0f84 Alexey Dobriyan   2009-12-14   96  	free_page((unsigned long)page);
0ead0f84 Alexey Dobriyan   2009-12-14   97  	return ret;
0ead0f84 Alexey Dobriyan   2009-12-14   98  }
^1da177e Linus Torvalds    2005-04-16   99  
0ead0f84 Alexey Dobriyan   2009-12-14  100  static int srm_env_proc_open(struct inode *inode, struct file *file)
0ead0f84 Alexey Dobriyan   2009-12-14  101  {
d9dda78b Al Viro           2013-03-31  102  	return single_open(file, srm_env_proc_show, PDE_DATA(inode));
^1da177e Linus Torvalds    2005-04-16  103  }
^1da177e Linus Torvalds    2005-04-16  104  
0ead0f84 Alexey Dobriyan   2009-12-14  105  static ssize_t srm_env_proc_write(struct file *file, const char __user *buffer,
0ead0f84 Alexey Dobriyan   2009-12-14  106  				  size_t count, loff_t *pos)
^1da177e Linus Torvalds    2005-04-16  107  {
^1da177e Linus Torvalds    2005-04-16 @108  	int res;
1c1ec6c6 Al Viro           2013-03-31  109  	unsigned long	id = (unsigned long)PDE_DATA(file_inode(file));
^1da177e Linus Torvalds    2005-04-16  110  	char		*buf = (char *) __get_free_page(GFP_USER);
^1da177e Linus Torvalds    2005-04-16  111  	unsigned long	ret1, ret2;
^1da177e Linus Torvalds    2005-04-16  112  
^1da177e Linus Torvalds    2005-04-16  113  	if (!buf)
^1da177e Linus Torvalds    2005-04-16  114  		return -ENOMEM;
^1da177e Linus Torvalds    2005-04-16  115  
cb234559 Markus Elfring    2017-01-18  116  	if (count >= PAGE_SIZE) {

:::::: The code at line 108 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12178 bytes --]