Re: [PATCH] MIPS: get_user: set the parameter @x to zero on error

[Ralf Baechle <ralf@linux-mips.org>]

On Tue, Nov 18, 2014 at 02:34:56PM +0800, bin.jiang@windriver.com wrote:

> From: Bin Jiang <bin.jiang@windriver.com>
> 
> The following compile warning is caused to use uninitialized variables:
> 
> fs/compat_ioctl.c: In function 'compat_SyS_ioctl':
> arch/mips/include/asm/uaccess.h:451:2: warning: 'length' may be used \
>                 uninitialized in this function [-Wmaybe-uninitialized]
>   __asm__ __volatile__(      \
>   ^
> fs/compat_ioctl.c:208:6: note: 'length' was declared here
>   int length, err;
>       ^
> 
> In get_user function, the parameter @x is used to store result. If the
> function return error, the @x won't be set and cause above warning.
> 
> According to the description of get_user function, the parameter @x should
> be set to zero on error.

You're not the first to send such a patch, see

  http://patchwork.linux-mips.org/patch/1307/

However I've hesistated to apply the previous patch which only claimed to
resolve a warning because __get_user and get_user get expanded very often
in the kernel so a small innocent looking change like this results in a
surprisingly large bloat.

A smart compiler will reorder this:

	int x;

	if (...) {
		...
	} else
		x = 0;

into:

	int x = 0;

	if (...) {
		...
	}

Which avoids the branches otherwise necessary for the else construct.  However
both the original and your patch fail to take care of the case where the
if is taken but __get_user_asm aborts due to an inaccessible fault.

That case is only fixed by manually doing above reordering - a compiler can't
know that the inline assembler won't assign anything in that case.

The comment btw was cut and paste and - blame me - it seems I failed to read
what it promises about @x for the error case; I had implemented get_user under
the assumption that the returned value was undefined in case of an -EFAULT
error.

Thanks for reporting this!

  Ralf