Messages in this thread | | | Date | Fri, 24 May 2019 18:00:15 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] misc: sgi-xp: Properly initialize buf in xpc_get_rsvd_page_pa |
| |
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
| |