Re: [PATCH v9 12/26] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

From: Thiago Macieira <thiago.macieira@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "Chang S. Bae" <chang.seok.bae@intel.com>, <luto@kernel.org>,
	<tglx@linutronix.de>, <mingo@kernel.org>, <x86@kernel.org>,
	<len.brown@intel.com>, <dave.hansen@intel.com>,
	<jing2.liu@intel.com>, <ravi.v.shankar@intel.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 12/26] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state
Date: Wed, 18 Aug 2021 10:58:42 -0700	[thread overview]
Message-ID: <4493449.UzBjrsCbmA@tjmaciei-mobl5> (raw)
In-Reply-To: <YR1HYRRN0HMTxXrw@zn.tnic>

On Wednesday, 18 August 2021 10:46:09 PDT Borislav Petkov wrote:
> You're new to this...
> 
> tools/arch/x86/kcpuid/kcpuid.c should be used for all CPUID querying
> needs.

That tells me what the CPU supports, not what the kernel does. By omitting the 
"xfd" entry in /proc/cpuinfo, we are assuming that all kernels with "amxtile" 
also implicitly support xfd. That is a valid assumption.

> I don't see a problem with the app doing at load time:
> 
> A: Heey, kernel, do you support AMX?
> K: Yes
> A: Allocate a dynamic FPU buffer for me then pls.

Many applications need to determine which plugins and code paths to enable 
before getting the data that will tell them what to do. It's entirely possible 
for them to never need to run the AMX instructions, so they may wish to defer 
the request to allocate the XSAVE state until they have read their input data.

It's indeed possible that the allocation then fails and the application be 
unable to continue. But OOM conditions are unlikely, so it may be an 
acceptable price to pay. In fact, by *not* allocating the extra state for 
every thread in the current process, it may avoid the OOM.

> > I was going to suggest a new API to return the supported bits, but
> > hadn't yet because it wasn't required for this patchset to work.
> 
> I think you should. The important part is having the API good and
> complete.
> 
> > So long as that API landed at or before the time a new bit was added,
> > userspace would be able to cope. But if the kernel is going to
> > allocate the bits at the moment of the system call *and* we wish for
> > userspace not to request more than it really needs, then we'll need
> > this extra API right now.
> 
> No no, once the API hits upstream, it is cast in stone. So it better
> be done in full with the patchset, in one go. No later significant API
> additions or changes, none especially after apps start using it.

Sorry, that's not what I meant. I was going to request an extra API, a third 
call. We'd have:
 - get current state
 - set new state
 - get available bits to set

The first two are in Chang's patch set, the third one is not. Right now, 
there's a single bit that can be set, so there's no need to have the third 
one. Any future software that wants to request a new bit will know if the 
kernel supports it by the very presence of the API. That is, if they ask and 
the API fails with -EINVAL, then this new bit isn't supported.

I didn't make the request because, as I said, it didn't seem required. 
Therefore, I didn't want to add further work before the minimum functionality 
got merged.

Now, if we are going to have this API any way, it might be a good idea to 
combine the two getters in one by adding a second pointer parameter.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel DPG Cloud Engineering