Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs

[Oleg Nesterov]

On 03/23, Thomas Gleixner wrote:
>
> On Thu, 28 Feb 2019, Gustavo A. R. Silva wrote:
> 
> >  arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> 
> Second thoughts. So this adds 28 /* fall through */ comments. Now I
> appreciate the effort, but can we pretty please look at the code in
> question and figure out whether the implementation makes sense in the first
> place before adding falltrough comments blindly?
> 
> The whole exercise can be simplified. Untested patch below.
> 
> Looking at that stuff makes me wonder about two things:
> 
>  1) The third argument of get/set(), i.e. the argument offset, is 0 on all
>     call sites. Do we need it at all?

Probably "maxargs" can be removed too, Steven sent the patches a long ago, see
https://lore.kernel.org/lkml/20161107212634.529267342@xxxxxxxxxxx/

>  2) syscall_set_arguments() has been introduced in 2008 and we still have
>     no caller. Instead of polishing it, can it be removed completely or are
>     there plans to actually use it?

I think it can die.

> 
> Thanks,
> 
> 	tglx
> 
> 8<----------------
> 
> arch/x86/include/asm/syscall.h |  174 +++++++++++++++--------------------------
>  1 file changed, 64 insertions(+), 110 deletions(-)
> 
> --- a/arch/x86/include/asm/syscall.h
> +++ b/arch/x86/include/asm/syscall.h
> @@ -114,126 +114,80 @@ static inline int syscall_get_arch(void)
>  
>  #else	 /* CONFIG_X86_64 */
>  
> +static inline unsigned long syscall_get_argreg(struct pt_regs *regs,
> +					       unsigned int idx)
> +{
> +	switch (idx) {
> +	case  0: return regs->di;
> +	case  1: return regs->si;
> +	case  2: return regs->dx;
> +	case  3: return regs->r10;
> +	case  4: return regs->r8;
> +	case  5: return regs->r9;
> +#ifdef CONFIG_IA32_EMULATION
> +	case  6: return regs->bx;
> +	case  7: return regs->cx;
> +	case  8: return regs->dx;
> +	case  9: return regs->si;
> +	case 10: return regs->di;
> +	case 11: return regs->bp;
> +#endif
> +	}
> +	return 0;
> +}
> +
>  static inline void syscall_get_arguments(struct task_struct *task,
>  					 struct pt_regs *regs,
> -					 unsigned int i, unsigned int n,
> +					 unsigned int idx, unsigned int cnt,
>  					 unsigned long *args)
>  {
> -# ifdef CONFIG_IA32_EMULATION
> -	if (task->thread_info.status & TS_COMPAT)
> -		switch (i) {
> -		case 0:
> -			if (!n--) break;
> -			*args++ = regs->bx;
> -		case 1:
> -			if (!n--) break;
> -			*args++ = regs->cx;
> -		case 2:
> -			if (!n--) break;
> -			*args++ = regs->dx;
> -		case 3:
> -			if (!n--) break;
> -			*args++ = regs->si;
> -		case 4:
> -			if (!n--) break;
> -			*args++ = regs->di;
> -		case 5:
> -			if (!n--) break;
> -			*args++ = regs->bp;
> -		case 6:
> -			if (!n--) break;
> -		default:
> -			BUG();
> -			break;
> -		}
> -	else
> -# endif
> -		switch (i) {
> -		case 0:
> -			if (!n--) break;
> -			*args++ = regs->di;
> -		case 1:
> -			if (!n--) break;
> -			*args++ = regs->si;
> -		case 2:
> -			if (!n--) break;
> -			*args++ = regs->dx;
> -		case 3:
> -			if (!n--) break;
> -			*args++ = regs->r10;
> -		case 4:
> -			if (!n--) break;
> -			*args++ = regs->r8;
> -		case 5:
> -			if (!n--) break;
> -			*args++ = regs->r9;
> -		case 6:
> -			if (!n--) break;
> -		default:
> -			BUG();
> -			break;
> -		}
> +	if (WARN_ON((idx + cnt) > 6))
> +		return;
> +
> +	if (IS_ENABLED(CONFIG_IA32_EMULATION) &&
> +	    task->thread_info.status & TS_COMPAT)
> +		idx += 6;
> +
> +	for (; cnt > 0; cnt--)
> +		*args++ = syscall_get_argreg(regs, idx++);
> +}
> +
> +static inline void syscall_set_argreg(struct pt_regs *regs,
> +				      unsigned int idx,
> +				      unsigned long val)
> +{
> +	switch (idx) {
> +	case  0: regs->di  = val; break;
> +	case  1: regs->si  = val; break;
> +	case  2: regs->dx  = val; break;
> +	case  3: regs->r10 = val; break;
> +	case  4: regs->r8  = val; break;
> +	case  5: regs->r9  = val; break;
> +#ifdef CONFIG_IA32_EMULATION
> +	case  6: regs->bx  = val; break;
> +	case  7: regs->cx  = val; break;
> +	case  8: regs->dx  = val; break;
> +	case  9: regs->si  = val; break;
> +	case 10: regs->di  = val; break;
> +	case 11: regs->bp  = val; break;
> +#endif
> +	}
>  }
>  
>  static inline void syscall_set_arguments(struct task_struct *task,
>  					 struct pt_regs *regs,
> -					 unsigned int i, unsigned int n,
> +					 unsigned int idx, unsigned int cnt,
>  					 const unsigned long *args)
>  {
> -# ifdef CONFIG_IA32_EMULATION
> -	if (task->thread_info.status & TS_COMPAT)
> -		switch (i) {
> -		case 0:
> -			if (!n--) break;
> -			regs->bx = *args++;
> -		case 1:
> -			if (!n--) break;
> -			regs->cx = *args++;
> -		case 2:
> -			if (!n--) break;
> -			regs->dx = *args++;
> -		case 3:
> -			if (!n--) break;
> -			regs->si = *args++;
> -		case 4:
> -			if (!n--) break;
> -			regs->di = *args++;
> -		case 5:
> -			if (!n--) break;
> -			regs->bp = *args++;
> -		case 6:
> -			if (!n--) break;
> -		default:
> -			BUG();
> -			break;
> -		}
> -	else
> -# endif
> -		switch (i) {
> -		case 0:
> -			if (!n--) break;
> -			regs->di = *args++;
> -		case 1:
> -			if (!n--) break;
> -			regs->si = *args++;
> -		case 2:
> -			if (!n--) break;
> -			regs->dx = *args++;
> -		case 3:
> -			if (!n--) break;
> -			regs->r10 = *args++;
> -		case 4:
> -			if (!n--) break;
> -			regs->r8 = *args++;
> -		case 5:
> -			if (!n--) break;
> -			regs->r9 = *args++;
> -		case 6:
> -			if (!n--) break;
> -		default:
> -			BUG();
> -			break;
> -		}
> +	if (WARN_ON((idx + cnt) > 6))
> +		return;
> +
> +	if (IS_ENABLED(CONFIG_IA32_EMULATION) &&
> +	    task->thread_info.status & TS_COMPAT)
> +		idx += 6;
> +
> +	for (; cnt > 0; cnt--)
> +		syscall_set_argreg(regs, idx++, *args++);
>  }
>  
>  static inline int syscall_get_arch(void)