linux-kernel - Re: [PATCH v10 06/28] x86/fpu/xstate: Add new variables to indicate dynamic XSTATE buffer size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <40B953C4-25C8-43F5-BF3D-33FA695FD7B2@intel.com>
Date:   Sun, 3 Oct 2021 22:36:13 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "bp@...e.de" <bp@...e.de>, "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Macieira, Thiago" <thiago.macieira@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH v10 06/28] x86/fpu/xstate: Add new variables to indicate
 dynamic XSTATE buffer size

On Oct 1, 2021, at 06:32, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> +/**
>> + * struct fpu_xstate_buffer_config - xstate buffer configuration
>> + * @max_size:			The CPUID-enumerated all-feature "maximum" size
>> + *				for xstate per-task buffer.
>> + * @min_size:			The size to fit into the statically-allocated
>> + *				buffer. With dynamic states, this buffer no longer
>> + *				contains all the enabled state components.
>> + * @user_size:			The size of user-space buffer for signal and
>> + *				ptrace frames, in the non-compacted format.
>> + */
> 
>> void fpstate_init(struct fpu *fpu)
>> {
>> 	union fpregs_state *state;
>> +	unsigned int size;
>> +	u64 mask;
>> 
>> -	if (likely(fpu))
>> +	if (likely(fpu)) {
>> 		state = &fpu->state;
>> -	else
>> +		/* The dynamic user states are not prepared yet. */
>> +		mask = xfeatures_mask_all & ~xfeatures_mask_user_dynamic;
> 
> The patch ordering is really odd. Why aren't you adding
> 
>     fpu->state_mask
> and
>     fpu->state_size
> 
> first and initialize state_mask and state_size to the fixed mode and
> then add the dynamic sizing on top?

I once considered the need of referencing fpu->state_size is not that much. At
runtime, the buffer re-allocation needs to calculate it. Then fpstate_init()
and the below are the ones that reference the size.

Maybe I was too conservative in adding this. I’m going to follow your
suggestion in a new patch.

>> 	/*
>> 	 * If the target FPU state is not resident in the CPU registers, just
>> 	 * memcpy() from current, else save CPU state directly to the target.
>> +	 *
>> +	 * KVM does not support dynamic user states yet. Assume the buffer
>> +	 * always has the minimum size.
>> 	 */
>> 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>> 		memcpy(&fpu->state, &current->thread.fpu.state,
>> -		       fpu_kernel_xstate_size);
>> +		       fpu_buf_cfg.min_size);
> 
> Which completely avoids the export of fpu_buf_cfg for KVM because the
> information is just available via struc fpu. As a bonus the export of
> fpu_kernel_xstate_size can be removed as well.
> 
> Hmm?

Yes, it is. But I suspect it needs to reference size values. KVM’s fpu data
can be allocated with the min size, instead of sizeof(struct fpu). KVM code
might want to know the max to support dynamic features, as similar to [1] or
so.

Thanks,
Chang

[1] https://lore.kernel.org/kvm/20210207154256.52850-4-jing2.liu@linux.intel.com/