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

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

[H. Peter Anvin]

Hi guys,

I was looking through the vdso setup code (because what I meant to be an
easy optimization turned out to be more "interesting" than expected...)

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.

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.)

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 */
                return;

        unsigned long offset = *ptr - from;
        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);

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

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

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

[H. Peter Anvin]

On July 21, 2025 3:02:25 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>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

Yes, it was more complicated than necessary, because at first I was looking at reusing the routine in some other places I was working on, but it doesn't seem to be necessary, so no reason to complicate things. 

Thanks for confirming what I suspected.