Re: [PATCH] KVM: x86: dynamically allocate large struct in em_fxrstor

[Nick Desaulniers]

On Wed, May 24, 2017 at 04:19:57PM +0200, Radim KrÄmÃÅ wrote:
> 2017-05-23 23:24-0700, Nick Desaulniers:
> > +	fx_state = kmalloc(sizeof(*fx_state), GFP_KERNEL);
> > +	fx_state = kmalloc(sizeof(*fx_state), GFP_KERNEL);
> 
> fx_state must be 16 byte aligned and x86 ARCH_KMALLOC_MINALIGN is 8, so
> this needs manual correction.

Radim, thanks for the code review. I appreciate it! I'm fairly new to
kernel patching, but I'd like to resolve this with your guidance.

Looking through the available functions in slab.h, it seems the only
function that allows specifying alignment is kmem_cache_create.

Is that the simplest solution?

If so, then it seems like a `struct kmem_cache *` should be added as a
member to `struct x86_emulate_ctxt`? I'm not sure yet where the context
gets initialized and destroyed.  Do you have any insight?

Also, is there a #define'd value to use for the 16B alignment that I
should be using?

> Also, please kmalloc also fxregs_state in fxrstor_fixup and em_fxsave so
> we again have only one storage type.

Then it should be easy to call kmem_cache_alloc() at these sites.

> > +	if (!fx_state)
> > +		return -ENOMEM;
> 
> The caller does not understand -ENOMEM.  The appropriate return value is
> X86EMUL_UNHANDLEABLE.

Ack.

> >  	if (ctxt->mode < X86EMUL_MODE_PROT64)
> > -		rc = fxrstor_fixup(ctxt, &fx_state);
> > +		rc = fxrstor_fixup(ctxt, fx_state);
> 
> Ah, fxrstor_fixup most likely got inlined and both of them put ~512 byte
> fxregs_state on the stack ... noinline attribute should solve the
> warning too.

While that would change fewer lines, doesn't the problem still exist in
the case of `ctxt->mode < X86EMUL_MODE_PROT64`, just split across two
stack frames?  As in shouldn't we still dynamically allocate fx_state?