linux-mips
[Top] [All Lists]

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

To: Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH 0/6] mips: diverse Makefile updates
From: Ralf Baechle <ralf@linux-mips.org>
Date: Tue, 1 Jun 2010 11:28:22 +0100
Cc: linux-mips <linux-mips@linux-mips.org>, Wu Zhangjin <wuzhangjin@gmail.com>
In-reply-to: <20100531105550.GA15995@merkur.ravnborg.org>
References: <20100530141939.GA22153@merkur.ravnborg.org> <20100530153939.GA22352@merkur.ravnborg.org> <20100530231954.GA318@linux-mips.org> <20100531102954.GA12669@linux-mips.org> <20100531105550.GA15995@merkur.ravnborg.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.20 (2009-08-17)
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

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH 0/6] mips: diverse Makefile updates, Ralf Baechle <=