[PATCH 1/2] x86: kvm: avoid -Wsometimes-uninitized warning

[Arnd Bergmann]

clang points out that running a 64-bit guest on a 32-bit host
would lead to uninitialized variables:

arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (!longmode) {
^~~~~~~~~
arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
^~~~~
arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
if (!longmode) {
^~~~~~~~~~~~~~~
arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
^
= 0
arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]

Since that combination is not supported anyway, change the condition
to tell the compiler how the code is actually executed.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---
arch/x86/kvm/hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index a39e38f13029..950436c502ba 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1607,7 +1607,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)

longmode = is_64_bit_mode(vcpu);

- if (!longmode) {
+ if (!IS_ENABLED(CONFIG_X86_64) || !longmode) {
param = ((u64)kvm_rdx_read(vcpu) << 32) |
(kvm_rax_read(vcpu) & 0xffffffff);
ingpa = ((u64)kvm_rbx_read(vcpu) << 32) |
--
2.20.0

[Arnd Bergmann]

clang finds a contruct suspicious that converts an unsigned
character to a signed integer and back, causing an overflow:

arch/x86/kvm/mmu.c:4605:39: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -205 to 51 [-Werror,-Wconstant-conversion]
u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
~~ ^~
arch/x86/kvm/mmu.c:4607:38: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -241 to 15 [-Werror,-Wconstant-conversion]
u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
~~ ^~
arch/x86/kvm/mmu.c:4609:39: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -171 to 85 [-Werror,-Wconstant-conversion]
u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
~~ ^~

Add an explicit cast to tell clang that everything works as
intended here.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

arch/x86/kvm/mmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 17ece7b994b1..aea7f969ecb8 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4602,11 +4602,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
*/

/* Faults from writes to non-writable pages */
- u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
+ u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
/* Faults from user mode accesses to supervisor pages */
- u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
+ u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
/* Faults from fetches of non-executable pages*/
- u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
+ u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
/* Faults from kernel mode fetches of user pages */
u8 smepf = 0;
/* Faults from kernel mode accesses of user pages */
--
2.20.0

[Sedat Dilek]

On Fri, Jul 12, 2019 at 11:12 AM Arnd Bergmann <ar...@arndb.de> wrote:
>
> clang finds a contruct suspicious that converts an unsigned
> character to a signed integer and back, causing an overflow:
>
> arch/x86/kvm/mmu.c:4605:39: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -205 to 51 [-Werror,-Wconstant-conversion]
> u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> ~~ ^~
> arch/x86/kvm/mmu.c:4607:38: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -241 to 15 [-Werror,-Wconstant-conversion]
> u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> ~~ ^~
> arch/x86/kvm/mmu.c:4609:39: error: implicit conversion from 'int' to 'u8' (aka 'unsigned char') changes value from -171 to 85 [-Werror,-Wconstant-conversion]
> u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> ~~ ^~
>
> Add an explicit cast to tell clang that everything works as
> intended here.
>

Feel free to add:

Link: https://github.com/ClangBuiltLinux/linux/issues/95
( See also patch proposal of Matthias Kaehlcke )

I had a different "simpler" approach to not see this anymore :-).
( See attached 2 patches )

- Sedat -

> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---
> arch/x86/kvm/mmu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 17ece7b994b1..aea7f969ecb8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4602,11 +4602,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
> */
>
> /* Faults from writes to non-writable pages */
> - u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> + u8 wf = (pfec & PFERR_WRITE_MASK) ? (u8)~w : 0;
> /* Faults from user mode accesses to supervisor pages */
> - u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> + u8 uf = (pfec & PFERR_USER_MASK) ? (u8)~u : 0;
> /* Faults from fetches of non-executable pages*/
> - u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> + u8 ff = (pfec & PFERR_FETCH_MASK) ? (u8)~x : 0;
> /* Faults from kernel mode fetches of user pages */
> u8 smepf = 0;
> /* Faults from kernel mode accesses of user pages */
> --
> 2.20.0
>

> --
> 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-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190712091239.716978-2-arnd%40arndb.de.

[Roman Kagan]

On Fri, Jul 12, 2019 at 11:12:29AM +0200, Arnd Bergmann wrote:
> clang points out that running a 64-bit guest on a 32-bit host
> would lead to uninitialized variables:
>
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> if (!longmode) {
> ^~~~~~~~~
> arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
> trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
> ^~~~~
> arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
> if (!longmode) {
> ^~~~~~~~~~~~~~~
> arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
> u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
> ^
> = 0
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>
> Since that combination is not supported anyway, change the condition
> to tell the compiler how the code is actually executed.

Hmm, the compiler *is* told all it needs:


arch/x86/kvm/x86.h:
...
static inline int is_long_mode(struct kvm_vcpu *vcpu)
{
#ifdef CONFIG_X86_64
return vcpu->arch.efer & EFER_LMA;
#else
return 0;
#endif
}

static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
{
int cs_db, cs_l;

if (!is_long_mode(vcpu))
return false;
kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
return cs_l;
}
...

so in !CONFIG_X86_64 case is_64_bit_mode() unconditionally returns
false, and the branch setting the values of the variables is always
taken.

> Signed-off-by: Arnd Bergmann <ar...@arndb.de>
> ---
> arch/x86/kvm/hyperv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index a39e38f13029..950436c502ba 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1607,7 +1607,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>
> longmode = is_64_bit_mode(vcpu);
>
> - if (!longmode) {
> + if (!IS_ENABLED(CONFIG_X86_64) || !longmode) {
> param = ((u64)kvm_rdx_read(vcpu) << 32) |
> (kvm_rax_read(vcpu) & 0xffffffff);
> ingpa = ((u64)kvm_rbx_read(vcpu) << 32) |

So this is rather a workaround for the compiler giving false positive.
I suggest to at least rephrase the log message to inidcate this.

Roman.

[Arnd Bergmann]

I think what happens here is that clang does not treat the return
code of track the return code of is_64_bit_mode() as a constant
expression, and therefore assumes that the if() condition
may or may not be true, for the purpose of determining whether
the variable is used without an inialization. This would hold even
if it later eliminates the code leading up to the if() in an optimization
stage. IS_ENABLED(CONFIG_X86_64) however is a constant
expression, so with the patch, it understands this.

In contrast, gcc seems to perform all the inlining first, and
then see if some variable is used uninitialized in the final code.
This gives additional information to the compiler, but makes the
outcome less predictable since it depends on optimization flags
and architecture specific behavior.

Both approaches have their own sets of false positive and false
negative warnings.

Arnd

[Paolo Bonzini]

On 12/07/19 15:02, Arnd Bergmann wrote:
> I think what happens here is that clang does not treat the return
> code of track the return code of is_64_bit_mode() as a constant
> expression, and therefore assumes that the if() condition
> may or may not be true, for the purpose of determining whether
> the variable is used without an inialization. This would hold even
> if it later eliminates the code leading up to the if() in an optimization
> stage. IS_ENABLED(CONFIG_X86_64) however is a constant
> expression, so with the patch, it understands this.
>
> In contrast, gcc seems to perform all the inlining first, and
> then see if some variable is used uninitialized in the final code.
> This gives additional information to the compiler, but makes the
> outcome less predictable since it depends on optimization flags
> and architecture specific behavior.
>
> Both approaches have their own sets of false positive and false
> negative warnings.

True, on the other hand constant returns are not really rocket science. :)

Maybe change is_long_mode to a macro if !CONFIG_X86_64? That would be
better if clang likes it.

Paolo

[Arnd Bergmann]

I had to also get rid of the temporary variable to make it work.
Sending v2 now.

Arnd

[Arnd Bergmann]

clang points out that running a 64-bit guest on a 32-bit host
would lead to uninitialized variables:

arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (!longmode) {
^~~~~~~~~
arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
^~~~~
arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
if (!longmode) {
^~~~~~~~~~~~~~~
arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
^
= 0
arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]

Since that combination is not supported anyway, change the condition
to tell the compiler how the code is actually executed.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

v2: make the change inside of is_64_bit_mode().
---
arch/x86/kvm/hyperv.c | 6 ++----
arch/x86/kvm/x86.h | 4 ++++
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index a39e38f13029..4c1c423a3d08 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1594,7 +1594,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
{

u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;

uint16_t code, rep_idx, rep_cnt;
- bool fast, longmode, rep;
+ bool fast, rep;

/*
* hypercall generates UD from non zero cpl and real mode
@@ -1605,9 +1605,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
return 1;
}

- longmode = is_64_bit_mode(vcpu);
-
- if (!longmode) {
+ if (!is_64_bit_mode(vcpu)) {

param = ((u64)kvm_rdx_read(vcpu) << 32) |
(kvm_rax_read(vcpu) & 0xffffffff);
ingpa = ((u64)kvm_rbx_read(vcpu) << 32) |

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index e08a12892e8b..b457b3267296 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -90,6 +90,7 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu)
#endif
}

+#ifdef CONFIG_X86_64

static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
{
int cs_db, cs_l;

@@ -99,6 +100,9 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)

kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
return cs_l;
}

+#else
+#define is_64_bit_mode(vcpu) false
+#endif

static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
{
--
2.20.0

[Roman Kagan]

Reviewed-by: Roman Kagan <rka...@virtuozzo.com>

However I still think the log message could state it more explicitly
that it was the compiler's fault, and the patch is a workaround for it.

Otherwise later on someone may decide to restore the similarity of
is_64_bit_mode() to other inlines in this file, and will be extremely
unlikely to test clang 32-bit build...

Roman.

[Arnd Bergmann]

Fair enough. I've reworded the changelog, as well as the patch to
document this now, in a way that should make it harder to introduce
the warning again by accident. Unfortunately, that #ifdef check
cannot be turned into an if(IS_ENABLED()) because kvm_r8_read()
is not defined on i386.

Note that the 0-day bot now tests with clang as well, so you would
definitely hear about a warning appearing.

v3 coming.

Arnd

[Arnd Bergmann]

Clang notices a code path in which some variables are never
initialized, but fails to figure out that this can never happen
on i386 because is_64_bit_mode() always returns false.

arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (!longmode) {
^~~~~~~~~
arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
^~~~~
arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
if (!longmode) {
^~~~~~~~~~~~~~~
arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
^
= 0
arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]

Flip the condition around to avoid the conditional execution on i386.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---

v3: reword commit log, simplify patch again

v2: make the change inside of is_64_bit_mode().
---

arch/x86/kvm/hyperv.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index a39e38f13029..c10a8b10b203 100644

--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1594,7 +1594,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
{

u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;

uint16_t code, rep_idx, rep_cnt;
- bool fast, longmode, rep;
+ bool fast, rep;

/*
* hypercall generates UD from non zero cpl and real mode

@@ -1605,9 +1605,14 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)

return 1;
}

- longmode = is_64_bit_mode(vcpu);
-
- if (!longmode) {

+#ifdef CONFIG_X86_64
+ if (is_64_bit_mode(vcpu)) {
+ param = kvm_rcx_read(vcpu);
+ ingpa = kvm_rdx_read(vcpu);
+ outgpa = kvm_r8_read(vcpu);
+ } else
+#endif
+ {

param = ((u64)kvm_rdx_read(vcpu) << 32) |
(kvm_rax_read(vcpu) & 0xffffffff);
ingpa = ((u64)kvm_rbx_read(vcpu) << 32) |

@@ -1615,13 +1620,6 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
outgpa = ((u64)kvm_rdi_read(vcpu) << 32) |
(kvm_rsi_read(vcpu) & 0xffffffff);
}
-#ifdef CONFIG_X86_64
- else {
- param = kvm_rcx_read(vcpu);
- ingpa = kvm_rdx_read(vcpu);
- outgpa = kvm_r8_read(vcpu);
- }
-#endif

code = param & 0xffff;
fast = !!(param & HV_HYPERCALL_FAST_BIT);
--
2.20.0

[Roman Kagan]

Reviewed-by: Roman Kagan <rka...@virtuozzo.com>

[Nathan Chancellor]

On Fri, Jul 12, 2019 at 04:13:09PM +0200, Arnd Bergmann wrote:

> Clang notices a code path in which some variables are never
> initialized, but fails to figure out that this can never happen
> on i386 because is_64_bit_mode() always returns false.
>
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> if (!longmode) {
> ^~~~~~~~~
> arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
> trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
> ^~~~~
> arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
> if (!longmode) {
> ^~~~~~~~~~~~~~~
> arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
> u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
> ^
> = 0
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>
> Flip the condition around to avoid the conditional execution on i386.
>
> Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Reviewed-by: Nathan Chancellor <natecha...@gmail.com>

[Paolo Bonzini]

Queued, thanks.

Paolo

[Paolo Bonzini]

On 12/07/19 11:12, Arnd Bergmann wrote:
> clang finds a contruct suspicious that converts an unsigned
> character to a signed integer and back, causing an overflow:

I like how the commit message conveys the braindead-ness of the warning.

Queued, thanks.

Paolo