Hi all,
When building with -Wuninitialized, 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.
I am not really sure how to properly initialize buf in this instance.
I would assume it would involve xpc_kmalloc_cacheline_aligned like
further down in the function but maybe not, this function isn't entirely
clear. Could we get your input, this is one of the last warnings I see
in a few allyesconfig builds.
Thanks,
Nathan
On Thu, May 02, 2019 at 08:33:40PM -0700, Nathan Chancellor wrote:
> Hi all,
>
> When building with -Wuninitialized, 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.
>
> I am not really sure how to properly initialize buf in this instance.
> I would assume it would involve xpc_kmalloc_cacheline_aligned like
> further down in the function but maybe not, this function isn't entirely
> clear. Could we get your input, this is one of the last warnings I see
> in a few allyesconfig builds.
>
> Thanks,
> Nathan
Hi all,
Friendly ping for comments/input. This is one of a few remaining
warnings I see, it'd be nice to get it fixed up so we can turn it on for
the whole kernel.
Cheers,
Nathan
On Wed, May 22, 2019 at 06:56:39PM -0700, Nathan Chancellor wrote:
> On Thu, May 02, 2019 at 08:33:40PM -0700, Nathan Chancellor wrote:
> > Hi all,
> >
> > When building with -Wuninitialized, 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.
> >
> > I am not really sure how to properly initialize buf in this instance.
> > I would assume it would involve xpc_kmalloc_cacheline_aligned like
> > further down in the function but maybe not, this function isn't entirely
> > clear. Could we get your input, this is one of the last warnings I see
> > in a few allyesconfig builds.
> >
> > Thanks,
> > Nathan
>
> Hi all,
>
> Friendly ping for comments/input. This is one of a few remaining
> warnings I see, it'd be nice to get it fixed up so we can turn it on for
> the whole kernel.
Patches are gladly welcome :)
thanks,
greg k-h
Hi Nathan,
Since this kind of self-initialization is considered undefined
behavior, the simplest fix here is to just initialize to NULL. It's a
reasonable interpretation of what is currently written, and will at
least make the existing code more deterministic.
Thanks,
Steve
On Wed, May 22, 2019 at 10:53 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Wed, May 22, 2019 at 06:56:39PM -0700, Nathan Chancellor wrote:
> > On Thu, May 02, 2019 at 08:33:40PM -0700, Nathan Chancellor wrote:
> > > Hi all,
> > >
> > > When building with -Wuninitialized, 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.
> > >
> > > I am not really sure how to properly initialize buf in this instance.
> > > I would assume it would involve xpc_kmalloc_cacheline_aligned like
> > > further down in the function but maybe not, this function isn't entirely
> > > clear. Could we get your input, this is one of the last warnings I see
> > > in a few allyesconfig builds.
> > >
> > > Thanks,
> > > Nathan
> >
> > Hi all,
> >
> > Friendly ping for comments/input. This is one of a few remaining
> > warnings I see, it'd be nice to get it fixed up so we can turn it on for
> > the whole kernel.
>
> Patches are gladly welcome :)
>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To post to this group, send email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190523055258.GC22946%40kroah.com.
> For more options, visit https://groups.google.com/d/optout.
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 <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
Thanks Steve for the suggestion, don't know why that never crossed my
mind...
I tried to follow buf all the way down in get_partition_rsvd_page to see
if there would be any dereferences and I didn't see any but I could
have easily missed something.
drivers/misc/sgi-xp/xpc_partition.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c
index 3eba1c420cc0..782ce95d3f17 100644
--- a/drivers/misc/sgi-xp/xpc_partition.c
+++ b/drivers/misc/sgi-xp/xpc_partition.c
@@ -70,7 +70,7 @@ xpc_get_rsvd_page_pa(int nasid)
unsigned long rp_pa = nasid; /* seed with nasid */
size_t len = 0;
size_t buf_len = 0;
- void *buf = buf;
+ void *buf = NULL;
void *buf_base = NULL;
enum xp_retval (*get_partition_rsvd_page_pa)
(void *, u64 *, unsigned long *, size_t *) =
--
2.22.0.rc1
Looks good to me. :) Since I don't contribute to the Linux kernel
directly, I appreciate you picking this up.
Thanks,
Steve
On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
<[email protected]> 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 <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
>
> Thanks Steve for the suggestion, don't know why that never crossed my
> mind...
>
> I tried to follow buf all the way down in get_partition_rsvd_page to see
> if there would be any dereferences and I didn't see any but I could
> have easily missed something.
>
> drivers/misc/sgi-xp/xpc_partition.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c
> index 3eba1c420cc0..782ce95d3f17 100644
> --- a/drivers/misc/sgi-xp/xpc_partition.c
> +++ b/drivers/misc/sgi-xp/xpc_partition.c
> @@ -70,7 +70,7 @@ xpc_get_rsvd_page_pa(int nasid)
> unsigned long rp_pa = nasid; /* seed with nasid */
> size_t len = 0;
> size_t buf_len = 0;
> - void *buf = buf;
> + void *buf = NULL;
> void *buf_base = NULL;
> enum xp_retval (*get_partition_rsvd_page_pa)
> (void *, u64 *, unsigned long *, size_t *) =
> --
> 2.22.0.rc1
>
On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
<[email protected]> 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 <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
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 <[email protected]>
Thanks Nathan for following up on this.
> ---
>
> Thanks Steve for the suggestion, don't know why that never crossed my
> mind...
>
> I tried to follow buf all the way down in get_partition_rsvd_page to see
> if there would be any dereferences and I didn't see any but I could
> have easily missed something.
>
> drivers/misc/sgi-xp/xpc_partition.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c
> index 3eba1c420cc0..782ce95d3f17 100644
> --- a/drivers/misc/sgi-xp/xpc_partition.c
> +++ b/drivers/misc/sgi-xp/xpc_partition.c
> @@ -70,7 +70,7 @@ xpc_get_rsvd_page_pa(int nasid)
> unsigned long rp_pa = nasid; /* seed with nasid */
> size_t len = 0;
> size_t buf_len = 0;
> - void *buf = buf;
> + void *buf = NULL;
> void *buf_base = NULL;
> enum xp_retval (*get_partition_rsvd_page_pa)
> (void *, u64 *, unsigned long *, size_t *) =
> --
> 2.22.0.rc1
>
--
Thanks,
~Nick Desaulniers
On Thu, May 23, 2019 at 8:05 PM 'Nick Desaulniers' via Clang Built
Linux <[email protected]> wrote:
>
> On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Nathan Chancellor <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
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 <[email protected]> wrote:
> >
> > On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
> > <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Nathan Chancellor <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
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
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.
Arnd's explanation during review:
/*
* 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.
Fixes: 279290294662 ("[IA64-SGI] cleanup the way XPC locates the reserved page")
Link: https://github.com/ClangBuiltLinux/linux/issues/466
Suggested-by: Stephen Hines <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
drivers/misc/sgi-xp/xpc_partition.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c
index 3eba1c420cc0..782ce95d3f17 100644
--- a/drivers/misc/sgi-xp/xpc_partition.c
+++ b/drivers/misc/sgi-xp/xpc_partition.c
@@ -70,7 +70,7 @@ xpc_get_rsvd_page_pa(int nasid)
unsigned long rp_pa = nasid; /* seed with nasid */
size_t len = 0;
size_t buf_len = 0;
- void *buf = buf;
+ void *buf = NULL;
void *buf_base = NULL;
enum xp_retval (*get_partition_rsvd_page_pa)
(void *, u64 *, unsigned long *, size_t *) =
--
2.22.0.rc1