On Tue, Sep 15, 2020 at 06:02:43PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 05:43:40PM +0200, Heiko Carstens wrote:
> > Address spaces still have to switched/changed for machines without the
> > mvcos instructions and especially for instructions like e.g. compare
> > and swap (-> futex) which must be executed in kernel address space but
> > access user address space. For such instructions enable_sacf_uaccess()
> > and disable_sacf_uaccess() must be used like before.
>
> That logic always confused me and still keeps confusing me,
> dumb questions below:
>
> > int oldval = 0, newval, ret;
> > - mm_segment_t old_fs;
> > + bool old;
> >
> > - old_fs = enable_sacf_uaccess();
> > + old = enable_sacf_uaccess();
> > switch (op) {
> > case FUTEX_OP_SET:
> > __futex_atomic_op("lr %2,%5\n",
> > @@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> > default:
> > ret = -ENOSYS;
> > }
> > - disable_sacf_uaccess(old_fs);
> > + disable_sacf_uaccess(old);
>
> Do we need to return the old value here? The way I understand it
> this is context switched with the thread, and given that only small
> isolated code bases now use it, sacf use can't nest, can it?
I just realized that this is broken for uaccess in irq context
(e.g. copy_from_user_nofault()). With set_fs() removal the calls to
force_uaccess_begin()/end() will do nothing, while before
set_fs(USER_DS) actually enforced that control registers on s390 were
setup correctly.
This wouldn't be the case anymore now. If e.g. a code sequence within
enable_sacf_uaccess() would be interrupted, and from within interrupt
context copy_from_user_nofault() would be executed, this would read
from kernel space instead from user space.
Needs fix.
