Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface

[Heiko Carstens <hca@linux.ibm.com>]

On Tue, Sep 15, 2020 at 06:02:43PM +0200, Christoph Hellwig wrote:
> >  	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?

Right, that should not happen. I think I'll change both functions to
void and add a WARN_ON_ONCE() to both of them, so that nested calls
will be catched.

> > @@ -116,7 +114,7 @@ struct thread_struct {
> >  	unsigned long hardirq_timer;	/* task cputime in hardirq context */
> >  	unsigned long softirq_timer;	/* task cputime in softirq context */
> >  	unsigned long sys_call_table;	/* system call table address */
> > -	mm_segment_t mm_segment;
> > +	bool sacf_uaccess;		/* uaccess with sacf enabled */
> 
> Isn't there a flags field somewhere where this could be folded into?

Hmm, a TIF flag will probably do.

> > -void set_fs_fixup(void)
> > -{
> > -	struct pt_regs *regs = current_pt_regs();
> > -	static bool warned;
> > -
> > -	set_fs(USER_DS);
> > -	if (warned)
> > -		return;
> > -	WARN(1, "Unbalanced set_fs - int code: 0x%x\n", regs->int_code);
> > -	show_registers(regs);
> > -	warned = true;
> 
> Would a warning about an unbalanced sacf flag still make sense?  Or
> just objtool for compile time checks similar to the unsafe uaccess
> routines on x86?

Yes, I was not sure to keep it or drop it. But now that you ask for
it, I'll add it back. FWIW, there is no objtool for s390 (yet).

> >  EXPORT_SYMBOL(enable_sacf_uaccess);
> 
> Neither enable_sacf_uaccess nor disable_sacf_uaccess appear to be
> used in modular code, so these exports can probably be dropped.

Ah, I wanted to check, and forgot that.