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
