MIME-Version: 1.0
References: <20230221184908.2349578-1-kpsingh@kernel.org> <Y/YJisQdorH1aAKV@zn.tnic>
 <CACYkzJ4cSA5xFScgS=WTc6tPis-vUCtYkh3LyEr8EkXoDCm-uA@mail.gmail.com>
 <Y/ZVaBKwbWUbF7u+@zn.tnic> <CACYkzJ4WigzaOCR4V9=e60ka=NNncWRB-j78DLRuzdSOZXvwrA@mail.gmail.com>
 <Y/dfyh1U/qDR1Ymw@zn.tnic>
In-Reply-To: <Y/dfyh1U/qDR1Ymw@zn.tnic>
From:   KP Singh <kpsingh@kernel.org>
Date:   Sat, 25 Feb 2023 20:50:23 -0500
Message-ID: <CACYkzJ6U9HbMCPbZTnLf5_vWzRwCxk5j74pOSAVt-26ZS_C=pw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] x86/speculation: Allow enabling STIBP with legacy IBRS
To:     Borislav Petkov <bp@alien8.de>
Cc:     linux-kernel@vger.kernel.org, pjt@google.com, evn@google.com,
        jpoimboe@kernel.org, tglx@linutronix.de, x86@kernel.org,
        hpa@zytor.com, peterz@infradead.org,
        pawan.kumar.gupta@linux.intel.com, kim.phillips@amd.com,
        alexandre.chartre@oracle.com, daniel.sneddon@linux.intel.com,
        corbet@lwn.net, bp@suse.de, linyujun809@huawei.com,
        jmattson@google.com,
        =?UTF-8?Q?Jos=C3=A9_Oliveira?= <joseloliveira11@gmail.com>,
        Rodrigo Branco <rodrigo@kernelhacking.com>,
        Alexandra Sandulescu <aesa@google.com>, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 23, 2023 at 7:45 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 22, 2023 at 11:41:59AM -0800, KP Singh wrote:
> > Sure, I think the docs do already cover it,
>
> I mean *our docs*. The stuff you're adding in your patch 2.
>
> > but I sort of disagree with your statement around the commit message.
> > I feel the more context you can add in the commit message, the better
> > it is.
>
> That's ofc wrong. And you'll find that out when you do git archeology
> and you come across a huuuge wall of text explaining the world and some
> more.
>
> No, commit messages should be to the point with a structure similar to
> something like this:
>
> 1. Prepare the context for the explanation briefly.
>
> 2. Explain the problem at hand.
>
> 3. "It happens because of <...>"
>
> 4. "Fix it by doing X"
>
> 5. "(Potentially do Y)."
>
> concentrating on *why* the fix is being done.
>
> > When I am looking at the change log, it would be helpful to have the
> > information that I mentioned in the Q&A. Small things like, "eIBRS
> > needs the IBRS bit set which also enables cross-thread protections" is
> > a very important context for this patch IMHO. Without this one is just
> > left head scratching and scrambling to read lengthy docs and processor
> > manuals.
>
> Yes, that's why you say in the commit message: "For more details, see
> Documentation/admin-guide/hw-vuln/spectre.rst." where:
>
> 1. you can explain in a lot more detail
>
> 2. put it in place where people can find it *easily*
>
> > This sort of loosely implies that the IBRS bit also enables
> > cross-thread protections. Can you atleast add this one explicitly?
> >
> > "Setting the IBRS bit also enables cross thread protections"
>
> Ok.
>
> > Not at the stage when the kernel decides to drop the STIBP protection
> > when eIBRS is enabled.
>
> We can't dump every possible interaction between the mitigations. It is
> a huge mess already. But I'm open to looking at improvements of the
> situation *and* documenting stuff as we go.
>

Well, we can try our best to print a message when we take a decision
on behalf of the user. As I mentioned, had I got any message that the
kernel was doing this it would have been much easier for me to figure
out why the POC was working. It was tricky without this to figure out
and I am capturing my broad chain of thought here:

1. cross thread training seems to work, this seems to be a bug
2. let me enable cross thread protections with a prctl
3. it still seems to work, did I get a printnk that my prctl did not
succeed or the kernel did something special? - no - why?

I will still go ahead with the patch that was "re-written" for me and
we can add debug information subsequently / as a follow up.

> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette