lkml.org 
[lkml]   [2023]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component
    Date
    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

    \
     
     \ /
      Last update: 2023-03-27 00:30    [W:4.552 / U:1.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site