Re: [PATCH] m68knommu: remove set_fs()

Hi Linus,

Am 08.07.2021 um 16:14 schrieb Linus Torvalds:
> On Wed, Jul 7, 2021 at 8:40 PM Michael Schmitz <[email protected]> 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.

OK, got it now.

> 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.

I can't recall any use of get_user() etc. in interrupt handlers, but 
that certainly warrants a closer look.

> 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.

I wonder whether we can end up scheduling in the return path from an 
interrupt (interrupts return via ret_from_exception on m68k, and that 
has a test for thread info flags relating to reschedule and signals)...

>
> 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.

Bit too subtle for me still ...

Cheers,

	Michael

>
>                   Linus
>