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

[Arnd Bergmann <arnd@arndb.de>]

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>