On Wed, Jul 7, 2021 at 8:40 PM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
going back to this one, I missed that bit earlier - the last three hunks
of your patch replaced KERNEL_DS by USER_DATA, everywhere else it's
replaced by SUPER_DATA. Typo, or something too subtle for me to grasp?
So I think the old KERNEL_DS was purely legacy, and isn't what I think
it's really supposed to be. It didn't _matter_, because then execve()
will set it to USER_DS by the time you run any user program, but I
didn't like it.
So I decided that in the new world order, the rules should be really
straightforward and simple:
- SFC/DFC is always USER_DATA normally, which is how get_user/put_user want it.
- special functions that actually use SFC/DFC for some temporary
override will set it to that temporary value, and then restore it to
USER_DATA after use.
and that's what I wrote my patch for.
BUT! And this is the important part:
My patch was completely untested garbage. I may have had opinions, I
may have had a plan, but the reality is that without testing (and
fixing things up for the things I had missed - like the code in
mm/maccess.c) that plan is just so much hot air.
In other words, take the above with a big pinch of salt.
And I want to stress once more time: if any of the SFC/DFC
modifications are done in interrupt handlers, that whole "set it back
to USER_DATA" is _wrong_. If they can happen in interrupts, then those
functions that modify SFC/DFC need to save the old value, and restore
it at the end, because otherwise you might have
- function that uses SFC/DFC:
set new 'value A'
<- get interrupt here
nested function that uses SFC/DFC
set new value B
.. do whatever special op
set SDC/DFC to USER_DATA
<- interrupt returns
original SFC/DFC function now has SFC/DFC with USER_DATA
it _wanted_ to have it with 'value A'
See the problem?
So if nesting can occur - due to interrupts - then all the things that
set a different SFC/DFC really need to save/restore the old one,
rather than set it back blindly to USER_DATA.
Also, finally: my patch had that "preempt_disable/preempt_enable"
around the SFC/DFC modifications. That was hot garbage. Christoph
correctly pointed out that switch_to() will save/restore SFC/DFC, so
there's no real reason to.
Except now that I think about it, I worry about getting scheduled away
*between* the instruction that sets SFC and the one that sets DFC. And
then switch_to() will save just SFC to the thread-struct. And then
restore the (new thread) SFC value to _both_ SFC and DFC.
So task switching doesn't actually save and restore SFC and DFC. It
only really saves SFC, and then it restores both SFC and DFC with the
same value.
Which should be ok, if the m68k code that modifies DFC will _always_
have modified SFC to the new value first. So even if we then schedule
away and back in between the two instructions, it only means that
we'll set DFC then to the value that it will soon be assigned anyway.
But it's all a tiny bit subtle and somewhat confusing.
Linus
