linux-kernel - Re: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial xstate data

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210209124906.GC15909@zn.tnic>
Date:   Tue, 9 Feb 2021 13:49:06 +0100
From:   Borislav Petkov <bp@...e.de>
To:     "Bae, Chang Seok" <chang.seok.bae@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "Hansen, Dave" <dave.hansen@...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>
Subject: Re: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial
 xstate data

On Mon, Feb 08, 2021 at 06:53:23PM +0000, Bae, Chang Seok wrote:
> Maybe, drop ‘for convenience’ from this sentence, since the buffer’s usage is
> not much relevant in this changelog.

Yes, "init_fpstate" is kinda clear what it is, from the name.

> It does functional change, but it is conditional to AMX enabling.
>
> It includes all the initial states when AMX states not enabled. But it will
> exclude the AMX state (with 8KB zeros) with the change.

Those sentences "no functional change" are supposed to mean that
the patch doesn't change anything and is only an equivalent code
transformation.

Yours does. So drop it from this one and from all the other patches as
it is causing more confusion than it is trying to dispel.

> I think they are in a different context.
> 
> The helper indicates the mask for the ‘init_fpstate’ buffer. The rest is the
> initial mask value for the per-task xstate buffer.

Wait, what?

Are you trying to tell me that that helper will return different masks
depending on xfeatures_mask_user_dynamic, which changes in its lifetime?

Then drop that helper altogether - that is more confusion and the xstate
code is already confusing enough.

> Since you suggested to introduce get_xstate_buffer_attr(), how about replacing
> what you found with something like:
> 
> get_xstate_buffer_attr(XSTATE_INIT_MASK)

I'd prefer no helper at all but only comments above the usage site.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg