Messages in this thread | | | Date | Fri, 22 Jun 2018 16:28:32 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v4 1/7] x86/fsgsbase/64: Introduce FS/GS base helper functions |
| |
On Wed, 20 Jun 2018, Chang S. Bae wrote: > +void write_fsbase(unsigned long fsbase) > +{ > + /* set the selector to 0 to not confuse __switch_to */
'to not confuse __switch_to' is not that helpful of a comment as it requires to stare into __switch_to to figure out what might get confused there. Please write it out why this needs to be done in technical terms.
> + loadseg(FS, 0); > + wrmsrl(MSR_FS_BASE, fsbase); > +} > + > +void write_inactive_gsbase(unsigned long gsbase) > +{ > + /* set the selector to 0 to not confuse __switch_to */
Ditto
> + loadseg(GS, 0); > + wrmsrl(MSR_KERNEL_GS_BASE, gsbase); > +} > + > +unsigned long read_task_fsbase(struct task_struct *task) > +{ > + unsigned long fsbase; > + > + if (task == current) { > + fsbase = read_fsbase(); > + } else { > + /* > + * XXX: This will not behave as expected if called > + * if fsindex != 0. This preserves an existing bug > + * that will be fixed.
I'm late to this party, but let me ask the obvious question:
Why is the existing bug not fixed as the first patch in the series?
We do not preserve bugs when adding new stuff as that makes it a pain to backport the fix.
> +int write_task_fsbase(struct task_struct *task, unsigned long fsbase) > +{ > + int cpu; > + > + /* > + * Not strictly needed for fs, but do it for symmetry > + * with gs > + */ > + if (unlikely(fsbase >= TASK_SIZE_MAX)) > + return -EPERM; > + > + cpu = get_cpu();
What's the point of using get_cpu()? There is nothing at all which needs 'cpu' here. The only point is to prevent preemption, then please use preempt_disable() and not some random function which happens to disable preemption underneath.
> + task->thread.fsbase = fsbase; > + if (task == current) > + write_fsbase(fsbase); > + task->thread.fsindex = 0; > + put_cpu(); > + > + return 0; > +} > + > +int write_task_gsbase(struct task_struct *task, unsigned long gsbase) > +{ > + int cpu; > + > + if (unlikely(gsbase >= TASK_SIZE_MAX)) > + return -EPERM; > + > + cpu = get_cpu();
Ditto
> + task->thread.gsbase = gsbase; > + if (task == current) > + write_inactive_gsbase(gsbase); > + task->thread.gsindex = 0; > + put_cpu(); > + > + return 0; > +} > + > int copy_thread_tls(unsigned long clone_flags, unsigned long sp, > unsigned long arg, struct task_struct *p, unsigned long tls) > { > @@ -618,54 +707,27 @@ static long prctl_map_vdso(const struct vdso_image *image, unsigned long addr) > long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2) > { > int ret = 0; > - int doit = task == current; > - int cpu; > > switch (option) { > - case ARCH_SET_GS: > - if (arg2 >= TASK_SIZE_MAX) > - return -EPERM; > - cpu = get_cpu();
Ah. You copied it from here where it makes no sense either. Copy and paste is useful, but you really want to think about it.
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > index e2ee403..c53c2bcf6 100644 > --- a/arch/x86/kernel/ptrace.c > +++ b/arch/x86/kernel/ptrace.c
....
> if (child->thread.fsbase != value) > - return do_arch_prctl_64(child, ARCH_SET_FS, value); > + return write_task_fsbase(child, value);
This patch wants to be split into:
1) Adding the new functions
2) Convert vdso
3) Convert ptrace
_AFTER_ fixing the existing bug.
Thanks,
tglx
| |