Re: [PATCH 0/6] mips: diverse Makefile updates

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

On Mon, May 31, 2010 at 12:55:50PM +0200, Sam Ravnborg wrote:

> > I played with it for a bit.  The warning is present in all gcc 4.1.0 to
> > 4.1.2 and it is bogus.  When I first looked into this years ago I just
> > gave up on gcc 4.1 as a newer version was already available.
> > 
> > The variable returned by get_user is undefined in case of an error, so
> > what get_user() is doing is entirely legitimate.  This is different from
> > copy_from_user() which in case of an error will clear the remainder of
> > the destination area which couldn't not be copied from userspace.
> 
> What I looked at:
> 
> 1)	u32 word;
> 2)	if (unlikely(get_user(word, header)))
> 3)		word = 0;
> 4)	if (word == magic.cmp)
> 5)		return PAGE_SIZE;
> 
> If gcc does not see an assignment to word in line 2) then
> it complains about the use line 4).
> 
> If we look at the implementation of get_user it is more or less the
> following:
> ({
> 	int err = -EFAULT;
> 	if (access_ok(VERIFY_READ, header))
> 		switch (sizeof(word)) {
> 		case 4:
> 			word = *(u32 *)header;
> 			err = 0;
> 			break;
> 		default:
> 			__get_user_unknown();
> 			break;
> 		}
> 	err;
> })
> 
> Simplified a lot - I know. But it shows my point.
> (And simplifying the code also helped me understand the macros).
> 
> gcc needs to be smart enough to deduce that we always return != 0
> in the cases where word is not assigned - in which case line 3)
> will do the assignment.
> 
> So gcc is indeed wrong when is sas "uninitialized" but considering
> the complexity of these macros I think it is excused.
> 
> The x86 version has the following assignment
> 
>     (val) = (__typeof__(*(addr))) __gu_tmp; 
> 
> unconditionally - so they avoid the " = 0" thing.
> sparc has explicit "= 0" assignments.
> 
> So refactoring the macros may do the trick too.
> But I do not think it is worth the effort.

One reason for the being written as they are is also that this allows a
compiler to generate some useful warnings such as:

	get_user(var, ptr);
	printk("var is %d\n", var);

Where var indeed can be used uninitialzed.  The return value of get_user()
being ignored is really a separate problem which should be attacked as
well.  __must_check would be the obvious way of doing this but it can only
be used as a function attribute.  Will have to experiment to see if it's
possible to use it within get_user / put_user in something like:

static inline int __must_check __gu_must_check(int __gu_err)
{
	return __gu_err
}

#define __get_user_check(x, ptr, size)                                  \
({                                                                      \
        int __gu_err = -EFAULT;                                         \
        const __typeof__(*(ptr)) __user * __gu_ptr = (ptr);             \
                                                                        \
        might_fault();                                                  \
        if (likely(access_ok(VERIFY_READ,  __gu_ptr, size)))            \
                __get_user_common((x), size, __gu_ptr);                 \
                                                                        \
        __gu_must_check(__gu_err);					\
})

  Ralf