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

[Nick Desaulniers]

On Fri, Jun 15, 2018 at 11:40 AM Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> On Fri, 2018-06-15 at 11:29 -0700, Matthias Kaehlcke wrote:
> > On Fri, Jun 15, 2018 at 11:18:12AM -0700, Joe Perches wrote:
> > > On Fri, 2018-06-15 at 11:04 -0700, Nick Desaulniers wrote:
> > > > On Fri, Jun 15, 2018 at 10:47 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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 are assigned to
> > > > > u8 variables. Cast the negated values 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)
> > >
> > > Perhaps it's better to turn off the warning.
> > > There are more of these in the kernel too.
> > >
> > > At least:
> > > drivers/regulator/max8660.c:        u8 mask = (rdev_get_id(rdev) == MAX8660_V3) ? ~1 : ~4;
> > > drivers/regulator/max8660.c:        u8 mask = (rdev_get_id(rdev) == MAX8660_V6) ? ~2 : ~4;
> > > fs/ext4/resize.c:   __u16 uninit_mask = (flexbg_size > 1) ? ~EXT4_BG_BLOCK_UNINIT : ~0;
> >
> > In my experience neither clang nor gcc should promote these values to
> > int, since the MSB/sign bit is not set.
>
> Well, that is what the c90 standard requires.
>
> 6.5.3.3 Unary arithmetic operators
> Constraints
> 4 The result of the ~ operator is the bitwise complement of its (promoted) operand (that is,
> each bit in the result is set if and only if the corresponding bit in the converted operand is
> not set). The integer promotions are performed on the operand, and the result has the
> promoted type. If the promoted type is an unsigned type, the expression ~E is equivalent
> to the maximum value representable in that type minus E.
>
> > In any case I think it it preferable to fix the code over disabling
> > the warning, unless the warning is bogus or there are just too many
> > occurrences.
>
> Maybe.

Spurious warning today, actual bug tomorrow?  I prefer to not to
disable warnings wholesale.  They don't need to find actual bugs to be
useful.  Flagging code that can be further specified does not hurt.
Part of the effort to compile the kernel with different compilers is
to add warning coverage, not remove it.  That said, there may be
warnings that are never useful (or at least due to some invariant that
only affects the kernel).  I cant think of any off the top of my head,
but I'm also not sure this is one.
-- 
Thanks,
~Nick Desaulniers