Re: [PATCH] misc: sgi-xp: Properly initialize buf in xpc_get_rsvd_page_pa

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, May 24, 2019 at 09:40:47AM +0200, Arnd Bergmann wrote:
> On Thu, May 23, 2019 at 8:05 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
> >
> > On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns:
> > >
> > > drivers/misc/sgi-xp/xpc_partition.c:73:14: warning: variable 'buf' is
> > > uninitialized when used within its own initialization [-Wuninitialized]
> > >         void *buf = buf;
> > >               ~~~   ^~~
> > > 1 warning generated.
> > >
> > > Initialize it to NULL, which is more deterministic.
> > >
> > > Fixes: 279290294662 ("[IA64-SGI] cleanup the way XPC locates the reserved page")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/466
> > > Suggested-by: Stephen Hines <srhines@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> >
> > From https://github.com/ClangBuiltLinux/linux/issues/466#issuecomment-488781917
> > I tried to follow the rabbit hole, but eventually these void* get
> > converted to u64's and passed along to function that I have no idea
> > whether they handle the value `(u64)(void*)0` or not.  Either way,
> > they definitely don't handle uninitialized values/UB.
> >
> > I was going to cc Robin who's already cc'ed, but looks like this code
> > was last touched 7-10 years ago. + Tony and Fenghua for ia64 since
> > sn_partition_reserved_page_pa is defined in
> > arch/ia64/include/asm/sn/sn_sal.h.
> >
> > In absence of consensus, I'll prefer NULL to uninitialized.
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > Thanks Nathan for following up on this.
> 
> I also had to take a look, and I think I understand what's going on,
> and interestingly, the code is correct, both before and after your patch.
> It's described in this comment:
> 
> /*
>  * Returns the physical address of the partition's reserved page through
>  * an iterative number of calls.
>  *
>  * On first call, 'cookie' and 'len' should be set to 0, and 'addr'
>  * set to the nasid of the partition whose reserved page's address is
>  * being sought.
>  * On subsequent calls, pass the values, that were passed back on the
>  * previous call.
>  *
>  * While the return status equals SALRET_MORE_PASSES, keep calling
>  * this function after first copying 'len' bytes starting at 'addr'
>  * into 'buf'. Once the return status equals SALRET_OK, 'addr' will
>  * be the physical address of the partition's reserved page. If the
>  * return status equals neither of these, an error as occurred.
>  */
> static inline s64
> sn_partition_reserved_page_pa(u64 buf, u64 *cookie, u64 *addr, u64 *len)
> 
> so *len is set to zero on the first call and tells the bios how many bytes
> are accessible at 'buf', and it does get updated by the BIOS to tell
> us how many bytes it needs, and then we allocate that and try again.
> 
> With that explanation added,
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Nathan, can you add this to the changelog comment and add the reviewed
by lines and resend it so I can queue it up?

thanks,

greg k-h