Re: [RFC PATCH v2 09/13] x86/um/vdso: nommu: vdso memory update

[Hajime Tazaki <thehajime@gmail.com>]

Thanks Benjamin,

On Wed, 27 Nov 2024 19:36:44 +0900,
Benjamin Berg wrote:

> > @@ -51,9 +65,15 @@ __kernel_old_time_t
> > __vdso_time(__kernel_old_time_t *t)
> >  {
> >  	long secs;
> >  
> > +#ifdef CONFIG_MMU
> >  	asm volatile("syscall"
> >  		: "=a" (secs)
> >  		: "0" (__NR_time), "D" (t) : "cc", "r11", "cx",
> > "memory");
> > +#else
> > +	asm("call *%1"
> > +		: "=a" (secs)
> > +		: "0" ((unsigned long)__NR_time), "D" (t) : "cc",
> > "r11", "cx", "memory");
> > +#endif
> 
> Maybe introduce a macro for "syscall" vs. "call *%1"? The parameters
> should be identical in both cases.

I think it's nice to clean up this part.

> The "call" could probably even jump
> to the end of the NOP ramp directly in this case.

ah, that would be a bit of speed ups.
confirmed that it works.  I'll update it if the code readability
doesn't become worse.

> Though maybe I am missing something with the "(unsigned long)" cast?

without this, gcc says

  CC      arch/x86/um/vdso/um_vdso.o
../arch/x86/um/vdso/um_vdso.c: Assembler messages:
../arch/x86/um/vdso/um_vdso.c:50: Error: operand size mismatch for `call'

so the cast intended to silence it.

but if I changed the constraint like below, the error is gone.

-		: "0" ((unsigned long)__NR_time), "D" (t) : "cc",
+		: "a" (__NR_time), "D" (t) : "cc",

I will clean it up as well.

> >  	return secs;
> >  }
> > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> > index f238f7b33cdd..83c861e2a815 100644
> > --- a/arch/x86/um/vdso/vma.c
> > +++ b/arch/x86/um/vdso/vma.c
> > @@ -9,6 +9,7 @@
> >  #include <asm/page.h>
> >  #include <asm/elf.h>
> >  #include <linux/init.h>
> > +#include <os.h>
> >  
> >  static unsigned int __read_mostly vdso_enabled = 1;
> >  unsigned long um_vdso_addr;
> > @@ -24,7 +25,9 @@ static int __init init_vdso(void)
> >  
> >  	BUG_ON(vdso_end - vdso_start > PAGE_SIZE);
> >  
> > +#ifdef CONFIG_MMU
> >  	um_vdso_addr = task_size - PAGE_SIZE;
> > +#endif
> >  
> >  	vdsop = kmalloc(sizeof(struct page *), GFP_KERNEL);
> >  	if (!vdsop)
> > @@ -40,6 +43,15 @@ static int __init init_vdso(void)
> >  	copy_page(page_address(um_vdso), vdso_start);
> >  	*vdsop = um_vdso;
> >  
> > +#ifndef CONFIG_MMU
> > +	/* this is fine with NOMMU as everything is accessible */
> > +	um_vdso_addr = (unsigned long)page_address(um_vdso);
> > +	os_protect_memory((void *)um_vdso_addr, vdso_end -
> > vdso_start, 1, 1, 1);
> 
> I think this should be "1, 0, 1", i.e. we shouldn't enable write
> access.

not sure if this relates to but with PROT_WRITE off I cannot put a
breakpoint with gdb on vdso area.  normal execution (without gdb)
looks fine.


-- Hajime