Re: Address fixups in arch/x86/entry/vdso/vma.c

[Thomas Gleixner]

On Thu, Jul 17 2025 at 10:34, H. Peter Anvin wrote:
> One of the thing that my mind flagged was this:
>
> static void vdso_fix_landing(const struct vdso_image *image,
>                 struct vm_area_struct *new_vma)
> {
>         if (in_ia32_syscall() && image == &vdso_image_32) {
>                 struct pt_regs *regs = current_pt_regs();
>                 unsigned long vdso_land = image->sym_int80_landing_pad;
>                 unsigned long old_land_addr = vdso_land +
>                         (unsigned long)current->mm->context.vdso;
>
>                 /* Fixing userspace landing - look at do_fast_syscall_32 */
>                 if (regs->ip == old_land_addr)
>                         regs->ip = new_vma->vm_start + vdso_land;
>         }
> }
>
> static int vdso_mremap(const struct vm_special_mapping *sm,
>                 struct vm_area_struct *new_vma)
> {
>         const struct vdso_image *image =
> 		current->mm->context.vdso_image;
>
>         vdso_fix_landing(image, new_vma);
>         current->mm->context.vdso = (void __user *)new_vma->vm_start;
>
>         return 0;
> }
>
>
> --- ---
>
> This feels *way* more complicated than it should need to be.  It seems
> to me that if the ip is inside the vdso at all, it would need to be
> adjusted, regardless of if it in an ia32 system call or not, and if it
> is at the specific landing spot or not.

In practice the only situation where ret->ip can be inside the VDSO is
when the remap syscall was invoked as IA32 syscall and the VDSO image is
a 32-bit image. So this check is pretty much paranoia.

> It is possible that it doesn't *matter*, but that's not really a good
> reason to make the code more complex.
>
> I came up with the following version as an alternative; I would be
> interesting to hear what you think.
>
> (Also, (unsigned long)current->mm->context.vdso occurs *all over the
> place*, but there is also a macro defined for it (VDSO_CURRENT_BASE, in
> <asm/elf.h>. My personal preference would be to replace both with an
> inline function.)

No objections, but in a seperate patch.

> If you don't think I'm missing something, I would like to do something
> like this:
>
>
> static inline void
> vdso_fix_address(unsigned long *ptr, const struct vdso_image *image,
>                  unsigned long from, unsigned long to)
> {
>         if (!image)		/* For potential uses elsewhere */

Aside of tail comments being horrible, this comment is just useless
gunk.

>                 return;
>
>         unsigned long offset = *ptr - from;

Why on earth do you need to hand in the pt_regs->ip pointer instead of
using pt_regs->ip here at the usage side? Just to make the code even
less understandable than the original one?

>         if (offset < image->size)
>                 *ptr = offset + to;
> }
>
> static int vdso_mremap(const struct vm_special_mapping *sm,
>                 struct vm_area_struct *new_vma)
> {
>         vdso_fix_address(&current_pt_regs()->ip,
>                          current->mm->context.vdso_image,
>                          vdso_current_base(),
>                          new_vma->vm_start);

TBH, this is incomprehensible garbage. If you want to simplify the whole
thing, then why not doing the obvious:

static int vdso_mremap(const struct vm_special_mapping *sm, struct vm_area_struct *new_vma)
{
	const struct vdso_image *image = current->mm->context.vdso_image;

        if (image) {
		struct pt_regs *regs = current_pt_regs();
        	unsigned long offset = regs->ip - vdso_current_base();

                /* Add a useful comment */
                if (offset < image->size)
                	regs->ip = new_vma->vm_start + offset;
        }

	current->mm->context.vdso = (void __user *)new_vma->vm_start;
}

Thanks,

        tglx