Re: [PATCH 2/4] KVM: nVMX: Properly pad 'struct kvm_vmx_nested_state_hdr'

[Sean Christopherson]

On Wed, May 05, 2021, Maxim Levitsky wrote:
> On Mon, 2021-05-03 at 17:08 +0200, Vitaly Kuznetsov wrote:
> > Eliminate the probably unwanted hole in 'struct kvm_vmx_nested_state_hdr':
> > 
> > Pre-patch:
> > struct kvm_vmx_nested_state_hdr {
> >         __u64                      vmxon_pa;             /*     0     8 */
> >         __u64                      vmcs12_pa;            /*     8     8 */
> >         struct {
> >                 __u16              flags;                /*    16     2 */
> >         } smm;                                           /*    16     2 */
> > 
> >         /* XXX 2 bytes hole, try to pack */
> > 
> >         __u32                      flags;                /*    20     4 */
> >         __u64                      preemption_timer_deadline; /*    24     8 */
> > };
> > 
> > Post-patch:
> > struct kvm_vmx_nested_state_hdr {
> >         __u64                      vmxon_pa;             /*     0     8 */
> >         __u64                      vmcs12_pa;            /*     8     8 */
> >         struct {
> >                 __u16              flags;                /*    16     2 */
> >         } smm;                                           /*    16     2 */
> >         __u16                      pad;                  /*    18     2 */
> >         __u32                      flags;                /*    20     4 */
> >         __u64                      preemption_timer_deadline; /*    24     8 */
> > };
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > ---
> >  arch/x86/include/uapi/asm/kvm.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 5a3022c8af82..0662f644aad9 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -437,6 +437,8 @@ struct kvm_vmx_nested_state_hdr {
> >  		__u16 flags;
> >  	} smm;
> >  
> > +	__u16 pad;
> > +
> >  	__u32 flags;
> >  	__u64 preemption_timer_deadline;
> >  };
> 
> 
> Looks good to me.
> 
> I wonder if we can enable the -Wpadded GCC warning to warn about such cases.
> Probably can't be enabled for the whole kernel but maybe we can enable it
> for KVM codebase at least, like we did with -Werror.

It'll never work, there are far, far too many structs throughout the kernel and
KVM that have implicit padding.  And for kernel-internal structs, that's perfectly
ok and even desirable since the kernel generally shouldn't make assumptions about
the layouts of its structs, i.e. it's a good thing the compiler pads structs so
that accesses are optimally aligned.

The padding behavior is only problematic for structs that are exposed to
userspace, because if userspace pads differently then we've got problems.  But
even then, building the kernel with -Wpadded wouldn't prevent userspace from
using a broken/goofy compiler that inserts unusual padding and misinterprets the
intended layout.

AFAIK, the C standard only expicitly disallows padding at the beginning of a
struct, i.e. the kernel's ABI is heavily reliant on existing compiler convention.
The only way to ensure exact layouts without relying on compiler convention would
be to tagged structs as packed, but "packed" also causes the compiler to generate
sub-optimal code since "packed" has strict requirements, and so the kernel relies
on sane compiler padding to provide a stable ABI.