Re: [PATCH 07/22] x86/fpu: Remove fpu->initialized

[Borislav Petkov]

On Thu, Feb 07, 2019 at 11:13:01AM +0100, Sebastian Andrzej Siewior wrote:
> and I *think* that this is enough. This *what* we do and not *why*. I
> don't have an answer towards *why*.

Well, it is a start.

You now have everything in your L1 and it is all clear but I'm sure all
the details will be LRU-evicted out soon :) and then you'd wish you'd
written down at least a small hint explaining the grand scheme at least.

> 
> > Considering that in this very thread we ourselves encountered the fact
> > that stuff is not documented and we complained that it wasn't!
> 
> Yes. We had no idea why we save the FPU registers on user's stack during
> signal handling. Was this an implementation detail on kernel side as
> part of signal handling or is this required/ expected by the user as
> part of a use case?

Well, at least a comment over get_sigframe() would've helped a long way,
right?

Instead of scratching heads why is this being done this way.

> We have now the explanation that signals may cascade. Do we know by
> now if userland is supposed to use it or it accessed the register
> because they were available? The MPX code did access the MPX part of
> the xsave area (others do it for "testing/debug" as per my I google
> research). This kind of things should be part of the ABI document and
> not only a comment in the kernel.

Absolutely agreed.

> Are the MAGIC constants only in-kernel use (to check if the user
> accidentally overwrote its stack) or should be checked by the user
> during signal handling to ensure that the xsave area is available.

I don't think the user should care but what do I know?!

> The part you referred to was:
> |-       /* Update the thread's fxstate to save the fsave header. */
> |-       if (ia32_fxstate) 
> |-               copy_fxregs_to_kernel(fpu);
> 
> and it is not helping because it does not explain why it is done. I can
> see based on the code that the FXstate is saved in case of a 32bit
> frame. It is saved into thread's state. It does not explain why it
> needs to be done. That is the "not helping" part.

This is *exactly* why I propose that we should have a
"grand-scheme-of-things" explanation somewhere about what we're doing
with the FPU context.

Figuring out what exactly to do in which context should be easier then.
I hope.

> I can forward you the IRC pieces offlist if you like. He said I can
> remove it if there are no users and I am not aware of any. He pointed
> out that sched_wakeup had a "success" member which was relied on by
> tools so it remained in order not to break them. So we have
> 	__entry->success        = 1; /* rudiment, kill when possible */
> 
> in the tree now. I can loop him in if this is not enough.

So you're replacing the old member with the new, AFAICT and I guess
that doesn't change offsets so even tools which don't use libtraceevent
should be fine but we better make sure before we break userspace because
we don't break userpace :)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.