[PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Matthias Kaehlcke]

update_permission_bitmask() negates u8 bitmask values and assigns them
to variables of type u8. Since the MSB is set in the bitmask values the
compiler expands the negated values to int, which then is assigned to
an u8 variable. Cast the negated value back to u8.

This fixes several warnings like this when building with clang:

arch/x86/kvm/mmu.c:4266: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;
       ~~                               ^~

(gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
doesn't seem to be universally enabled)

Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- negate the bitmask at initialization and rename variables to not_X

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

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d634f0332c0f..ad0a8c35f27b 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4258,8 +4258,9 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
 {
 	unsigned byte;
 
-	const u8 x = BYTE_MASK(ACC_EXEC_MASK);
-	const u8 w = BYTE_MASK(ACC_WRITE_MASK);
+	const u8 not_x = (u8)~BYTE_MASK(ACC_EXEC_MASK);
+	const u8 not_w = (u8)~BYTE_MASK(ACC_WRITE_MASK);
+	const u8 not_u = (u8)~BYTE_MASK(ACC_USER_MASK);
 	const u8 u = BYTE_MASK(ACC_USER_MASK);
 
 	bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
@@ -4275,11 +4276,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) ? not_w : 0;
 		/* Faults from user mode accesses to supervisor pages */
-		u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
+		u8 uf = (pfec & PFERR_USER_MASK) ? not_u : 0;
 		/* Faults from fetches of non-executable pages*/
-		u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
+		u8 ff = (pfec & PFERR_FETCH_MASK) ? not_x : 0;
 		/* Faults from kernel mode fetches of user pages */
 		u8 smepf = 0;
 		/* Faults from kernel mode accesses of user pages */
-- 
2.18.0.rc2.346.g013aa6912e-goog

RE: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[David Laight]

From: Matthias Kaehlcke
> Sent: 19 June 2018 20:25
> To: Paolo Bonzini; Radim Krčmář; Thomas Gleixner; H . Peter Anvin
> Cc: x86@kernel.org; kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Nick Desaulniers; Joe Perches;
> Matthias Kaehlcke
> Subject: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()
> 
> update_permission_bitmask() negates u8 bitmask values and assigns them
> to variables of type u8. Since the MSB is set in the bitmask values the
> compiler expands the negated values to int, which then is assigned to
> an u8 variable. Cast the negated value back to u8.
> 
> This fixes several warnings like this when building with clang:
> 
> arch/x86/kvm/mmu.c:4266: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;
>        ~~                               ^~

Shoot the compiler writer - or turn off that warning.
Or try 'w ^ 0xff'.

	David

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Paolo Bonzini]

On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> update_permission_bitmask() negates u8 bitmask values and assigns them
> to variables of type u8. Since the MSB is set in the bitmask values the
> compiler expands the negated values to int, which then is assigned to
> an u8 variable. Cast the negated value back to u8.
> 
> This fixes several warnings like this when building with clang:
> 
> arch/x86/kvm/mmu.c:4266: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;
>        ~~                               ^~
> 
> (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> doesn't seem to be universally enabled)
> 
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - negate the bitmask at initialization and rename variables to not_X

The patch is not that bad, but I'd like to get confirmation that other
maintainers are applying fixes like this.  Honestly I'm not really
impressed by most new clang warnings, these days.

Paolo

>  arch/x86/kvm/mmu.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index d634f0332c0f..ad0a8c35f27b 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4258,8 +4258,9 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  {
>  	unsigned byte;
>  
> -	const u8 x = BYTE_MASK(ACC_EXEC_MASK);
> -	const u8 w = BYTE_MASK(ACC_WRITE_MASK);
> +	const u8 not_x = (u8)~BYTE_MASK(ACC_EXEC_MASK);
> +	const u8 not_w = (u8)~BYTE_MASK(ACC_WRITE_MASK);
> +	const u8 not_u = (u8)~BYTE_MASK(ACC_USER_MASK);
>  	const u8 u = BYTE_MASK(ACC_USER_MASK);
>  
>  	bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
> @@ -4275,11 +4276,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) ? not_w : 0;
>  		/* Faults from user mode accesses to supervisor pages */
> -		u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> +		u8 uf = (pfec & PFERR_USER_MASK) ? not_u : 0;
>  		/* Faults from fetches of non-executable pages*/
> -		u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> +		u8 ff = (pfec & PFERR_FETCH_MASK) ? not_x : 0;
>  		/* Faults from kernel mode fetches of user pages */
>  		u8 smepf = 0;
>  		/* Faults from kernel mode accesses of user pages */
> 

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Nick Desaulniers]

On Mon, Jun 25, 2018 at 12:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> > update_permission_bitmask() negates u8 bitmask values and assigns them
> > to variables of type u8. Since the MSB is set in the bitmask values the
> > compiler expands the negated values to int, which then is assigned to
> > an u8 variable. Cast the negated value back to u8.
> >
> > This fixes several warnings like this when building with clang:
> >
> > arch/x86/kvm/mmu.c:4266: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;
> >        ~~                               ^~
> >
> > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > doesn't seem to be universally enabled)
> >
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - negate the bitmask at initialization and rename variables to not_X
>
> The patch is not that bad, but I'd like to get confirmation that other
> maintainers are applying fixes like this.  Honestly I'm not really
> impressed by most new clang warnings, these days.

Here's an actual bug this warning caught applied to drivers/input/:

dae1a432ab1f ("Input: mousedev - fix implicit conversion warning"):
https://patchwork.kernel.org/patch/9753771/

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Paolo Bonzini]

On 25/06/2018 18:47, Nick Desaulniers wrote:
> Here's an actual bug this warning caught applied to drivers/input/:
> 
> dae1a432ab1f ("Input: mousedev - fix implicit conversion warning"):
> https://patchwork.kernel.org/patch/9753771/
> 

How does the warning catch a bug there?  While I do prefer the code
after the patch, wouldn't everything work just fine with "signed char
ps2[6]".

So it seems to me in that case (unlike KVM) the warning caught some ugly
code, but not a bug.  (In fact if there was a bug, it should have been
mentioned in the commit message, but it wasn't).

Paolo

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Joe Perches]

On Mon, 2018-06-25 at 12:47 -0400, Nick Desaulniers wrote:
> On Mon, Jun 25, 2018 at 12:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> > > update_permission_bitmask() negates u8 bitmask values and assigns them
> > > to variables of type u8. Since the MSB is set in the bitmask values the
> > > compiler expands the negated values to int, which then is assigned to
> > > an u8 variable. Cast the negated value back to u8.
> > > 
> > > This fixes several warnings like this when building with clang:
> > > 
> > > arch/x86/kvm/mmu.c:4266: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;
> > >        ~~                               ^~
> > > 
> > > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > > doesn't seem to be universally enabled)
> > > 
> > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > Changes in v2:
> > > - negate the bitmask at initialization and rename variables to not_X
> > 
> > The patch is not that bad, but I'd like to get confirmation that other
> > maintainers are applying fixes like this.  Honestly I'm not really
> > impressed by most new clang warnings, these days.
> 
> Here's an actual bug this warning caught applied to drivers/input/:
> 
> dae1a432ab1f ("Input: mousedev - fix implicit conversion warning"):
> https://patchwork.kernel.org/patch/9753771/

What bug is that?

$ cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <memory.h>

int main(int argc, char **argv)
{
	static const signed char a[3] = {0x60, 3, 200};
	static const unsigned char b[3] = {0x60, 3, 200};

	printf("a and b are %s\n",
	       memcmp(a, b, 3) == 0 ? "identical" : "different");
}
$ gcc test.c
$ ./a.out
a and b are identical

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Nick Desaulniers]

On Mon, Jun 25, 2018 at 1:05 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2018-06-25 at 12:47 -0400, Nick Desaulniers wrote:
> > On Mon, Jun 25, 2018 at 12:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> > > > update_permission_bitmask() negates u8 bitmask values and assigns them
> > > > to variables of type u8. Since the MSB is set in the bitmask values the
> > > > compiler expands the negated values to int, which then is assigned to
> > > > an u8 variable. Cast the negated value back to u8.
> > > >
> > > > This fixes several warnings like this when building with clang:
> > > >
> > > > arch/x86/kvm/mmu.c:4266: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;
> > > >        ~~                               ^~
> > > >
> > > > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > > > doesn't seem to be universally enabled)
> > > >
> > > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > > Changes in v2:
> > > > - negate the bitmask at initialization and rename variables to not_X
> > >
> > > The patch is not that bad, but I'd like to get confirmation that other
> > > maintainers are applying fixes like this.  Honestly I'm not really
> > > impressed by most new clang warnings, these days.
> >
> > Here's an actual bug this warning caught applied to drivers/input/:
> >
> > dae1a432ab1f ("Input: mousedev - fix implicit conversion warning"):
> > https://patchwork.kernel.org/patch/9753771/
>
> What bug is that?
>
> $ cat test.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <memory.h>
>
> int main(int argc, char **argv)
> {
>         static const signed char a[3] = {0x60, 3, 200};
>         static const unsigned char b[3] = {0x60, 3, 200};
>
>         printf("a and b are %s\n",
>                memcmp(a, b, 3) == 0 ? "identical" : "different");
> }
> $ gcc test.c
> $ ./a.out
> a and b are identical
>

Good point, poor choice of example on my part.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Joe Perches]

On Mon, 2018-06-25 at 13:12 -0400, Nick Desaulniers wrote:
> On Mon, Jun 25, 2018 at 1:05 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2018-06-25 at 12:47 -0400, Nick Desaulniers wrote:
[]
> > > Here's an actual bug this warning caught applied to drivers/input/:
> > > 
> > > dae1a432ab1f ("Input: mousedev - fix implicit conversion warning"):
> > > https://patchwork.kernel.org/patch/9753771/
> > 
> > What bug is that?
> > 
> > $ cat test.c
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <memory.h>
> > 
> > int main(int argc, char **argv)
> > {
> >         static const signed char a[3] = {0x60, 3, 200};
> >         static const unsigned char b[3] = {0x60, 3, 200};
> > 
> >         printf("a and b are %s\n",
> >                memcmp(a, b, 3) == 0 ? "identical" : "different");
> > }
> > $ gcc test.c
> > $ ./a.out
> > a and b are identical
> > 
> Good point, poor choice of example on my part.

Is there an actual example of this so-called "bug"
in the kernel code?

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Matthias Kaehlcke]

Hi Paolo,

On Mon, Jun 25, 2018 at 06:05:01PM +0200, Paolo Bonzini wrote:
> On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> > update_permission_bitmask() negates u8 bitmask values and assigns them
> > to variables of type u8. Since the MSB is set in the bitmask values the
> > compiler expands the negated values to int, which then is assigned to
> > an u8 variable. Cast the negated value back to u8.
> > 
> > This fixes several warnings like this when building with clang:
> > 
> > arch/x86/kvm/mmu.c:4266: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;
> >        ~~                               ^~
> > 
> > (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> > doesn't seem to be universally enabled)
> > 
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - negate the bitmask at initialization and rename variables to not_X
> 
> The patch is not that bad, but I'd like to get confirmation that other
> maintainers are applying fixes like this.  Honestly I'm not really
> impressed by most new clang warnings, these days.

Some other instances of the warning that have been addressed are:

commit 644d87dccdc69cf79834a72ed0c889580d6af32a
Author: Stefan Agner <stefan@agner.ch>
Date:   Thu Apr 5 16:25:38 2018 -0700

    mm/memblock.c: cast constant ULLONG_MAX to phys_addr_t


commit e2a5dca753d1cdc3212519023ed8a13e13f5495b
Author: Borislav Petkov <bp@suse.de>
Date:   Thu Nov 23 10:19:51 2017 +0100

    x86/umip: Fix insn_get_code_seg_params()'s return value


commit dae1a432ab1fe79ae53129ededeaece35a2dc14d
Author: Nick Desaulniers <nick.desaulniers@gmail.com>
Date:   Sat Jun 24 22:50:12 2017 -0700

    Input: mousedev - fix implicit conversion warning


commit d1600401faad4bc186bfdb291d8af644465e20bd
Author: Matthias Kaehlcke <mka@chromium.org>
Date:   Fri Mar 31 18:00:04 2017 -0700

    ALSA: hda/ca0132: Limit values for chip addresses to 32-bit


commit a45463cbf3f9dcdae683033c256f50bded513d6a
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Wed Feb 1 18:01:17 2017 +0100

    workqueue: avoid clang warning


Thanks

Matthias

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Joe Perches]

On Mon, 2018-06-25 at 10:35 -0700, Matthias Kaehlcke wrote:
> Some other instances of the warning that have been addressed are:
> commit 644d87dccdc69cf79834a72ed0c889580d6af32a
> commit e2a5dca753d1cdc3212519023ed8a13e13f5495b
> commit dae1a432ab1fe79ae53129ededeaece35a2dc14d
> commit d1600401faad4bc186bfdb291d8af644465e20bd
> commit a45463cbf3f9dcdae683033c256f50bded513d6a
 
I think all these implicit conversion warnings are senseless
and should really be silenced by setting them only to be
output at W=3 with all the other non-static and generally
unnecessary signed/unsigned conversions

---
 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 8d5357053f86..9b1d0d59e8d0 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -46,6 +46,7 @@ warning-2 += $(call cc-option, -Wunused-macros)
 warning-3 := -Wbad-function-cast
 warning-3 += -Wcast-qual
 warning-3 += -Wconversion
+warning-3 += $(call cc-option -Wconstant-conversion)
 warning-3 += -Wpacked
 warning-3 += -Wpadded
 warning-3 += -Wpointer-arith

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Nick Desaulniers]

On Mon, Jun 25, 2018 at 1:50 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2018-06-25 at 10:35 -0700, Matthias Kaehlcke wrote:
> > Some other instances of the warning that have been addressed are:
> > commit 644d87dccdc69cf79834a72ed0c889580d6af32a
> > commit e2a5dca753d1cdc3212519023ed8a13e13f5495b
> > commit dae1a432ab1fe79ae53129ededeaece35a2dc14d
> > commit d1600401faad4bc186bfdb291d8af644465e20bd
> > commit a45463cbf3f9dcdae683033c256f50bded513d6a
>
> I think all these implicit conversion warnings are senseless
> and should really be silenced by setting them only to be
> output at W=3 with all the other non-static and generally
> unnecessary signed/unsigned conversions
>
> ---
>  scripts/Makefile.extrawarn | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 8d5357053f86..9b1d0d59e8d0 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -46,6 +46,7 @@ warning-2 += $(call cc-option, -Wunused-macros)
>  warning-3 := -Wbad-function-cast
>  warning-3 += -Wcast-qual
>  warning-3 += -Wconversion
> +warning-3 += $(call cc-option -Wconstant-conversion)
>  warning-3 += -Wpacked
>  warning-3 += -Wpadded
>  warning-3 += -Wpointer-arith

I prefer that to turning off the warning outright.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

[Paolo Bonzini]

On 25/06/2018 19:35, Matthias Kaehlcke wrote:
> commit e2a5dca753d1cdc3212519023ed8a13e13f5495b
> Author: Borislav Petkov <bp@suse.de>
> Date:   Thu Nov 23 10:19:51 2017 +0100
> 
>     x86/umip: Fix insn_get_code_seg_params()'s return value

So this is the closest thing to a bug (it could have been a real bug
if the caller checked ret < 0 instead of ret == -EINVAL).  *However*
the warning there is a bit misleading too.

The compiler does absolutely nothing to tell you that the return value
is in the [-EINVAL,132] range and therefore it should have been int.
_That_ would have been a useful warning.  Instead, it says:

      arch/x86/lib/insn-eval.c:780:10: warning: implicit conversion from 'int' to 'char'
              changes value from 132 to -124 [-Wconstant-conversion]
                      return INSN_CODE_SEG_PARAMS(4, 8);
                      ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~
      ./arch/x86/include/asm/insn-eval.h:16:57: note: expanded from macro 'INSN_CODE_SEG_PARAMS'
      #define INSN_CODE_SEG_PARAMS(oper_sz, addr_sz) (oper_sz | (addr_sz << 4))
    
If you changed the return value from char to u8, you'd break the "return
-EINVAL;" case.  The compiler would probably complain in turn that this
is not the right fix, by signaling a warning on "return -EINVAL;", but the
warning could really be done better.

Thanks,

Paolo