Messages in this thread | | | Date | Tue, 26 Mar 2019 16:12:45 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2] x86/syscalls: Mark expected switch fall-throughs |
| |
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@goodmis.org/
> 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)
| |