-Wuninitialized warning in drivers/misc/sgi-xp/xpc_partition.c

-Wuninitialized warning in drivers/misc/sgi-xp/xpc_partition.c

[Nathan Chancellor]

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

Re: -Wuninitialized warning in drivers/misc/sgi-xp/xpc_partition.c

[Nathan Chancellor]

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

Re: -Wuninitialized warning in drivers/misc/sgi-xp/xpc_partition.c

[Greg Kroah-Hartman]

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

Re: -Wuninitialized warning in drivers/misc/sgi-xp/xpc_partition.c

[Stephen Hines]

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
<gregkh@linuxfoundation.org> 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 clang-built-linux+unsubscribe@googlegroups.com.
> To post to this group, send email to clang-built-linux@googlegroups.com.
> 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.

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

[Nathan Chancellor]

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>
---

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

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

[Stephen Hines]

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
<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>
> ---
>
> 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
>

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

[Nick Desaulniers]

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.

> ---
>
> 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

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

[Arnd Bergmann]

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>

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

[Greg Kroah-Hartman]

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

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

[Nathan Chancellor]

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 <srhines@google.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 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