Re: [PATCH v12 10/18] x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions

[Thomas Gleixner]

Sasha,

Sasha Levin <sashal@xxxxxxxxxx> writes:
> Thank you for taking the time to review this.

welcome and sorry for the explosion.

> On Mon, May 18, 2020 at 08:20:08PM +0200, Thomas Gleixner wrote:
>>Sasha Levin <sashal@xxxxxxxxxx> writes:
>>This conditional irqsave gunk is clearly NOT what was in the tip tree
>>before it got reverted:
>>
>>  a86b4625138d ("x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions")
>
> It wasn't in the reverted series, it came in Intel's v9 series, with
> these comments in the cover letter:
>
> 	Updates from v8 [10]:
> 	[...]
> 	* Simplified GS base helper functions (Tony L.)

Ok. I never looked at that series because that requested confirmation
that nothing will regress due to the ptrace changes was not there. After
a bit of handwaving this dried out. So I completely missed that back
then. And I did not look at any later variant which had 0day complaints.

> And I did, Thomas. While I'm not intimately familiar with the code I
> made sure that all the patches that came on top of the merged series
> before it got reverted made it into this new series. However, more work
> has happened here after the revert and I would expect that the code in
> this new series will be different than the code you reverted last
> year.

It's obvious that it would be different from what was merged simply
because the affected code has changed but not in substantial points like
losing a kprobes protection by "simplifying" something which was
carefully done in the first place.

It's not your fault at all, you just happened to be the messanger. The
people responsible for that mess owe you at least a beer.

>> - In paranoid_exit()
>>
>>-	TRACE_IRQS_IRETQ_DEBUG
>>+	TRACE_IRQS_OFF_DEBUG
>
> (assuming we're looking at the same thing here, ) Changed in v8 of the
> series.

Sigh.

> I'm a bit confused about the surprise here that v12 is different than
> the reverted patches. There were multiple rounds of review which
> resulted in the code being more than just a revert of the revert along
> with a small fix.
>
>> > This looks unpleasant to review. ÂI wonder if it would be better to unrevert
>> > the reversion, merge up to Linus' tree or -tip, and then base the changes on
>> > top of that.
>>
>> I don't think that's a good idea. The old code is broken in several ways
>> and not bisectable. So we really better start from scratch.
>
> And this is what we have here, a series that has more than trivial
> differences from the revert, and is more of a pain to review. Look at
> what you did with your 25 minutes: you've reverted the revert and went
> on to apply fixes on top of it, exactly the thing you've asked
> not to do a few months prior.

I did that to analyse whether that new series has everything what was
fixed back then and did not introduce new bugs. Mission accomplished.

> No need to worry about me sending a new series, as I can't - I just
> don't know what you want to see at this point: on one hand you're saying
> "we really better start from scratch" and on the other hand "this
> conditional irqsave gunk is clearly NOT what was in the tip tree before
> it got reverted", you're making suggestions to change comments only to
> later complain that "a gazillion of comment changes make reading the
> diff harder". 

Gah. That comment change thing was just an annoyance and I complained
about it because I was already grumpy as hell.

So what I meant is that the blind revert of the revert, i.e. just
reapplying the previous stuff, is horrible. Simply because the reverted
patches were already not bisectable. And then applying random changes on
top does not make it any better.

So yes, I would have done exactly where I started:

   1) Extract the original patches from git

   2) Apply them and fixup the rejects

and on top of that:

   3) Make them bisectable by folding back the fixes to the right place
      and reordering them which creates a result which is equivalent to
      'start from scratch' but without losing context and introducing
      new bugs. Simply because it's trivial to diff against the state
      before the revert.

   4) Do the 'improvements' on top, discuss them and fold them back.

For what you tried to do I would have omitted #4 completely and then
did:

   5) Rebase the latest Intel variant

   6) Diff the results ideally step by step

   7) Analyze the deltas carefully and if unsure about the result
      ask.
      
   That way you really would have noticed that this helper patch is
   substantially different and you would have noticed that the kprobes
   protection is gone. Also that would have clearly shown you the IRQ
   flag wreckage.

So to go forward can you please just do #1 - #3 first?

Vs. the s/GSBASE/GS base/g comment changes: I don't mind them per se,
but they are incomplete because they just change it in the new code
while there are still the original comments using GSBASE. So either we
change it wholesale or not at all. If so, then this wants to be a
separate patch right at the beginning of the new series which changes
the existing comments before introducing a different variant.

That "simplified" handling is going nowhere. That conditional irq
disable and the redundant conditionals and the out of line invocation in
switch_to() are just not going to happen.

So when comparing it to the latest Intel trainwreck ignore that part
completely,

I've uploaded my quick shot with a few cleanups on top (folded back) for
reference:

  https://tglx.de/~tglx/patches-fsgs.tar

Uncompiled and untested. I'm not claiming it's bug free either. If you
find one, please keep it. Hope that helps.

Thanks,

        tglx