From:   Thomas Gleixner <tglx@linutronix.de>
To:     Mingwei Zhang <mizhang@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
        Mingwei Zhang <mizhang@google.com>,
        Jim Mattson <jmattson@google.com>,
        Venkatesh Srinivas <venkateshs@google.com>,
        Aaron Lewis <aaronlewis@google.com>,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Chao Gao <chao.gao@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address
 of init_fpstate if fpstate contains the component
In-Reply-To: <20230221163655.920289-2-mizhang@google.com>
References: <20230221163655.920289-1-mizhang@google.com>
 <20230221163655.920289-2-mizhang@google.com>
Date:   Tue, 21 Feb 2023 21:54:40 +0100
Message-ID: <878rgqoi27.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Mingwei!

On Tue, Feb 21 2023 at 16:36, Mingwei Zhang wrote:
> Avoid getting xstate address of init_fpstate if fpstate contains the xstate
> component. Since XTILEDATA (bit 18) was turned off in xinit, when KVM calls
> __raw_xsave_addr(xinit, 18), it triggers a warning as follows.

This does not parse. Aside of that it gets the ordering of the changelog
wrong. It explain first what the patch is doing by repeating the way too
long subject line and then tries to give some explanation about the
problem.

KVM does not call __raw_xsave_addr() and the problem is completely
independent of KVM.

> __raw_xsave_addr() is an internal function that assume caller does the
> checking, ie., all function arguments should be checked before calling.
> So, instead of removing the WARNING, add checks in
> __copy_xstate_to_uabi_buf().

I don't see checks added. The patch open codes copy_feature() and makes
the __raw_xsave_addr() invocations conditional.

So something like this:

   Subject: x86/fpu/xstate: Prevent false-positive warning in __copy_xstate_to_uabi_buf()

   __copy_xstate_to_uabi_buf() copies either from the tasks XSAVE buffer
   or from init_fpstate into the ptrace buffer. Dynamic features, like
   XTILEDATA, have an all zeroes init state and are not saved in
   init_fpstate, which means the corresponding bit is not set in the
   xfeatures bitmap of the init_fpstate header.

   But __copy_xstate_to_uabi_buf() retrieves addresses for both the
   tasks xstate and init_fpstate unconditionally via __raw_xsave_addr().

   So if the tasks XSAVE buffer has a dynamic feature set, then the
   address retrieval for init_fpstate triggers the warning in
   __raw_xsave_addr() which checks the feature bit in the init_fpstate
   header.

   Prevent this by open coding copy_feature() and making the address
   retrieval conditional on the tasks XSAVE header bit.

So the order here is (in paragraphs):

   1) Explain the context
   2) Explain whats wrong
   3) Explain the consequence
   4) Explain the solution briefly

It does not even need a backtrace, because the backtrace does not give
any relevant information which is not in the text above already.

The actual callchain is completely irrelevant for desribing this
issue. If you really want to add a backtrace, then please trim it by
removing the irrelevant information. See
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces
for further information.

So for this case this would be:

WARNING: CPU: 35 PID: 15304 at arch/x86/kernel/fpu/xstate.c:934 __raw_xsave_addr+0xc8/0xe0
Call Trace:
 <TASK>
 __copy_xstate_to_uabi_buf+0x3cb/0x520
 fpu_copy_guest_fpstate_to_uabi+0x29/0x50

But even fpu_copy_guest_fpstate_to_uabi() is already useless because
__copy_xstate_to_uabi_buf() has multiple callers which all have the very
same problem and they are very simple to find.

Backtraces are useful to illustrate a hard to understand callchain, but
having ~40 lines of backtrace which only contains two relevant lines is
just a distraction.

See?

> Fixes: e84ba47e313d ("x86/fpu: Hook up PKRU into ptrace()")

The problem did not exist at this point in time because dynamic
xfeatures did not exist, neither in hardware nor in kernel support.

Even if dynamic features would have existed then the commit would not be
the one which introduced the problem, All the commit does is to move
back then valid code into a conditional code path.

It fixes:

  471f0aa7fa64 ("x86/fpu: Fix copy_xstate_to_uabi() to copy init states correctly")

which attempted to fix exactly the problem you are seeing, but only
addressed half of it. The underlying problem was introduced with
2308ee57d93d ("x86/fpu/amx: Enable the AMX feature in 64-bit mode")

Random fixes tags are not really helpful.

> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>  			pkru.pkru = pkru_val;
>  			membuf_write(&to, &pkru, sizeof(pkru));
>  		} else {
> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
> -				     __raw_xsave_addr(xsave, i),
> -				     __raw_xsave_addr(xinit, i),
> -				     xstate_sizes[i]);

Please add a comment here to explain why this is open coded and does not
use copy_feature(). Something like:

    			/*
                         * Open code copy_feature() to prevent retrieval
                         * of a dynamic features address from xinit, which
                         * is invalid because xinit does not contain the
                         * all zeros init states of dynamic features and
                         * emits a warning.
                         */

> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
> +				__raw_xsave_addr(xsave, i) :
> +				__raw_xsave_addr(xinit, i);
> +
> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);

Other than that. Nice detective work!

Thanks,

        tglx