LoongArch: Add support to clone a time namespace

  When execute the following command to test clone3 on LoongArch:

  # cd tools/testing/selftests/clone3 && make && ./clone3

we can see the following error info:

  # [5719] Trying clone3() with flags 0x80 (size 0)
  # Invalid argument - Failed to create new process
  # [5719] clone3() with flags says: -22 expected 0
  not ok 18 [5719] Result (-22) is different than expected (0)

This is because if CONFIG_TIME_NS is not set, but the flag
CLONE_NEWTIME (0x80) is used to clone a time namespace, it
will return -EINVAL in copy_time_ns().

Here is the related code in include/linux/time_namespace.h:

  #ifdef CONFIG_TIME_NS
  ...
  struct time_namespace *copy_time_ns(unsigned long flags,
				      struct user_namespace *user_ns,
				      struct time_namespace *old_ns);
  ...
  #else
  ...
  static inline
  struct time_namespace *copy_time_ns(unsigned long flags,
				      struct user_namespace *user_ns,
				      struct time_namespace *old_ns)
  {
	  if (flags & CLONE_NEWTIME)
		  return ERR_PTR(-EINVAL);

	  return old_ns;
  }
  ...
  #endif

Here is the complete call stack:

  clone3()
    kernel_clone()
      copy_process()
        copy_namespaces()
          create_new_namespaces()
            copy_time_ns()
              clone_time_ns()

Because CONFIG_TIME_NS depends on GENERIC_VDSO_TIME_NS, select
GENERIC_VDSO_TIME_NS to enable CONFIG_TIME_NS to build the real
implementation of copy_time_ns() in kernel/time/namespace.c.

Additionally, it needs to define some arch dependent functions
such as __arch_get_timens_vdso_data(), arch_get_vdso_data() and
vdso_join_timens(), then the failed test can be fixed.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

This is based on 6.4-rc2

 arch/loongarch/Kconfig                         |  1 +
 arch/loongarch/include/asm/vdso/gettimeofday.h |  7 ++++++
 arch/loongarch/kernel/vdso.c                   | 32 ++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
  

Hi, Tiezhu,

The layout of vdso data (loongarch_vdso_data):

       struct vdso_pcpu_data pdata[NR_CPUS];
       struct vdso_data data[CS_BASES];

VDSO_DATA_SIZE is the page aligned size of loongarch_vdso_data, and in
memory, vdso code is above vdso data.

Then, get_vdso_base() returns the start of vdso code, and
get_vdso_data() returns the start of vdso data.

In your patch, __arch_get_timens_vdso_data() returns get_vdso_data() +
PAGE_SIZE, but you don't increase the size of loongarch_vdso_data. The
result is it returns an address in vdso code.

Now, do you know what the problem is? Or still insist that "I have tested"?

Huacai

On Wed, May 17, 2023 at 11:03 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> When execute the following command to test clone3 on LoongArch:
>
>   # cd tools/testing/selftests/clone3 && make && ./clone3
>
> we can see the following error info:
>
>   # [5719] Trying clone3() with flags 0x80 (size 0)
>   # Invalid argument - Failed to create new process
>   # [5719] clone3() with flags says: -22 expected 0
>   not ok 18 [5719] Result (-22) is different than expected (0)
>
> This is because if CONFIG_TIME_NS is not set, but the flag
> CLONE_NEWTIME (0x80) is used to clone a time namespace, it
> will return -EINVAL in copy_time_ns().
>
> Here is the related code in include/linux/time_namespace.h:
>
>   #ifdef CONFIG_TIME_NS
>   ...
>   struct time_namespace *copy_time_ns(unsigned long flags,
>                                       struct user_namespace *user_ns,
>                                       struct time_namespace *old_ns);
>   ...
>   #else
>   ...
>   static inline
>   struct time_namespace *copy_time_ns(unsigned long flags,
>                                       struct user_namespace *user_ns,
>                                       struct time_namespace *old_ns)
>   {
>           if (flags & CLONE_NEWTIME)
>                   return ERR_PTR(-EINVAL);
>
>           return old_ns;
>   }
>   ...
>   #endif
>
> Here is the complete call stack:
>
>   clone3()
>     kernel_clone()
>       copy_process()
>         copy_namespaces()
>           create_new_namespaces()
>             copy_time_ns()
>               clone_time_ns()
>
> Because CONFIG_TIME_NS depends on GENERIC_VDSO_TIME_NS, select
> GENERIC_VDSO_TIME_NS to enable CONFIG_TIME_NS to build the real
> implementation of copy_time_ns() in kernel/time/namespace.c.
>
> Additionally, it needs to define some arch dependent functions
> such as __arch_get_timens_vdso_data(), arch_get_vdso_data() and
> vdso_join_timens(), then the failed test can be fixed.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>
> This is based on 6.4-rc2
>
>  arch/loongarch/Kconfig                         |  1 +
>  arch/loongarch/include/asm/vdso/gettimeofday.h |  7 ++++++
>  arch/loongarch/kernel/vdso.c                   | 32 ++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index d38b066..93b167f 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -80,6 +80,7 @@ config LOONGARCH
>         select GENERIC_SCHED_CLOCK
>         select GENERIC_SMP_IDLE_THREAD
>         select GENERIC_TIME_VSYSCALL
> +       select GENERIC_VDSO_TIME_NS
>         select GPIOLIB
>         select HAS_IOPORT
>         select HAVE_ARCH_AUDITSYSCALL
> diff --git a/arch/loongarch/include/asm/vdso/gettimeofday.h b/arch/loongarch/include/asm/vdso/gettimeofday.h
> index 7b2cd37..1af88ac 100644
> --- a/arch/loongarch/include/asm/vdso/gettimeofday.h
> +++ b/arch/loongarch/include/asm/vdso/gettimeofday.h
> @@ -94,6 +94,13 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
>         return get_vdso_data();
>  }
>
> +#ifdef CONFIG_TIME_NS
> +static __always_inline
> +const struct vdso_data *__arch_get_timens_vdso_data(const struct vdso_data *vd)
> +{
> +       return get_vdso_data() + PAGE_SIZE;
> +}
> +#endif
>  #endif /* !__ASSEMBLY__ */
>
>  #endif /* __ASM_VDSO_GETTIMEOFDAY_H */
> diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
> index eaebd2e..cf62103 100644
> --- a/arch/loongarch/kernel/vdso.c
> +++ b/arch/loongarch/kernel/vdso.c
> @@ -14,6 +14,7 @@
>  #include <linux/random.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/time_namespace.h>
>  #include <linux/timekeeper_internal.h>
>
>  #include <asm/page.h>
> @@ -73,6 +74,37 @@ static int __init init_vdso(void)
>  }
>  subsys_initcall(init_vdso);
>
> +#ifdef CONFIG_TIME_NS
> +struct vdso_data *arch_get_vdso_data(void *vvar_page)
> +{
> +       return (struct vdso_data *)(vvar_page);
> +}
> +
> +/*
> + * The vvar mapping contains data for a specific time namespace, so when a
> + * task changes namespace we must unmap its vvar data for the old namespace.
> + * Subsequent faults will map in data for the new namespace.
> + *
> + * For more details see timens_setup_vdso_data().
> + */
> +int vdso_join_timens(struct task_struct *task, struct time_namespace *ns)
> +{
> +       struct mm_struct *mm = task->mm;
> +       struct vm_area_struct *vma;
> +
> +       VMA_ITERATOR(vmi, mm, 0);
> +
> +       mmap_read_lock(mm);
> +       for_each_vma(vmi, vma) {
> +               if (vma_is_special_mapping(vma, &vdso_info.data_mapping))
> +                       zap_vma_pages(vma);
> +       }
> +       mmap_read_unlock(mm);
> +
> +       return 0;
> +}
> +#endif
> +
>  static unsigned long vdso_base(void)
>  {
>         unsigned long base = STACK_TOP;
> --
> 2.1.0
>
  

On 05/18/2023 10:25 AM, Huacai Chen wrote:
> Hi, Tiezhu,
>
> The layout of vdso data (loongarch_vdso_data):
>
>        struct vdso_pcpu_data pdata[NR_CPUS];
>        struct vdso_data data[CS_BASES];
>
> VDSO_DATA_SIZE is the page aligned size of loongarch_vdso_data, and in
> memory, vdso code is above vdso data.
>
> Then, get_vdso_base() returns the start of vdso code, and
> get_vdso_data() returns the start of vdso data.
>
> In your patch, __arch_get_timens_vdso_data() returns get_vdso_data() +
> PAGE_SIZE, but you don't increase the size of loongarch_vdso_data. The
> result is it returns an address in vdso code.
>
> Now, do you know what the problem is? Or still insist that "I have tested"?

Please review the following changes based on the current patch,
modify the layout of vvar to expand a page size for timens_data,
and also map it to zero pfn before creating time namespace, then
the last thing is to add the callback function vvar_fault().

$ git diff
diff --git a/arch/loongarch/include/asm/page.h 
b/arch/loongarch/include/asm/page.h
index fb5338b..26e8dcc 100644
--- a/arch/loongarch/include/asm/page.h
+++ b/arch/loongarch/include/asm/page.h
@@ -81,6 +81,7 @@ typedef struct { unsigned long pgprot; } pgprot_t;
  #define __va(x)                ((void *)((unsigned long)(x) + 
PAGE_OFFSET - PHYS_OFFSET))

  #define pfn_to_kaddr(pfn)      __va((pfn) << PAGE_SHIFT)
+#define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))

  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
index cf62103..3e89aca 100644
--- a/arch/loongarch/kernel/vdso.c
+++ b/arch/loongarch/kernel/vdso.c
@@ -23,7 +23,27 @@
  #include <vdso/vsyscall.h>
  #include <generated/vdso-offsets.h>

+/*
+ * The layout of vvar:
+ *
+ *                 high
+ * +----------------+----------------+
+ * | timens_data    | PAGE_SIZE      |
+ * +----------------+----------------+
+ * | vdso_data      |                |
+ * | vdso_pcpu_data | VDSO_DATA_SIZE |
+ * +----------------+----------------+
+ *                 low
+ */
+#define VVAR_SIZE (VDSO_DATA_SIZE + PAGE_SIZE)
+
+enum vvar_pages {
+       VVAR_DATA_PAGE_OFFSET,
+       VVAR_TIMENS_PAGE_OFFSET,
+};
+
  extern char vdso_start[], vdso_end[];
+extern unsigned long zero_pfn;

  /* Kernel-provided data used by the VDSO. */
  static union {
@@ -42,6 +62,40 @@ static int vdso_mremap(const struct 
vm_special_mapping *sm, struct vm_area_struc
         return 0;
  }

+static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
+                            struct vm_area_struct *vma, struct vm_fault 
*vmf)
+{
+       struct page *timens_page = find_timens_vvar_page(vma);
+       unsigned long pfn;
+
+       switch (vmf->pgoff) {
+       case VVAR_DATA_PAGE_OFFSET:
+               if (timens_page)
+                       pfn = page_to_pfn(timens_page);
+               else
+                       pfn = sym_to_pfn(vdso_data);
+               break;
+#ifdef CONFIG_TIME_NS
+       case VVAR_TIMENS_PAGE_OFFSET:
+               /*
+                * If a task belongs to a time namespace then a namespace
+                * specific VVAR is mapped with the 
VVAR_DATA_PAGE_OFFSET and
+                * the real VVAR page is mapped with the 
VVAR_TIMENS_PAGE_OFFSET
+                * offset.
+                * See also the comment near timens_setup_vdso_data().
+                */
+               if (!timens_page)
+                       return VM_FAULT_SIGBUS;
+               pfn = sym_to_pfn(vdso_data);
+               break;
+#endif /* CONFIG_TIME_NS */
+       default:
+               return VM_FAULT_SIGBUS;
+       }
+
+       return vmf_insert_pfn(vma, vmf->address, pfn);
+}
+
  struct loongarch_vdso_info vdso_info = {
         .vdso = vdso_start,
         .size = PAGE_SIZE,
@@ -52,6 +106,7 @@ struct loongarch_vdso_info vdso_info = {
         },
         .data_mapping = {
                 .name = "[vvar]",
+               .fault = vvar_fault,
         },
         .offset_sigreturn = vdso_offset_sigreturn,
  };
@@ -120,7 +175,7 @@ static unsigned long vdso_base(void)
  int arch_setup_additional_pages(struct linux_binprm *bprm, int 
uses_interp)
  {
         int ret;
-       unsigned long vvar_size, size, data_addr, vdso_addr;
+       unsigned long size, data_addr, vdso_addr;
         struct mm_struct *mm = current->mm;
         struct vm_area_struct *vma;
         struct loongarch_vdso_info *info = current->thread.vdso;
@@ -132,17 +187,16 @@ int arch_setup_additional_pages(struct 
linux_binprm *bprm, int uses_interp)
          * Determine total area size. This includes the VDSO data itself
          * and the data pages.
          */
-       vvar_size = VDSO_DATA_SIZE;
-       size = vvar_size + info->size;
+       size = VVAR_SIZE + info->size;

         data_addr = get_unmapped_area(NULL, vdso_base(), size, 0, 0);
         if (IS_ERR_VALUE(data_addr)) {
                 ret = data_addr;
                 goto out;
         }
-       vdso_addr = data_addr + VDSO_DATA_SIZE;
+       vdso_addr = data_addr + VVAR_SIZE;

-       vma = _install_special_mapping(mm, data_addr, vvar_size,
+       vma = _install_special_mapping(mm, data_addr, VVAR_SIZE,
                                        VM_READ | VM_MAYREAD,
                                        &info->data_mapping);
         if (IS_ERR(vma)) {
@@ -153,7 +207,12 @@ int arch_setup_additional_pages(struct linux_binprm 
*bprm, int uses_interp)
         /* Map VDSO data page. */
         ret = remap_pfn_range(vma, data_addr,
                               virt_to_phys(&loongarch_vdso_data) >> 
PAGE_SHIFT,
-                             vvar_size, PAGE_READONLY);
+                             VDSO_DATA_SIZE, PAGE_READONLY);
+       if (ret)
+               goto out;
+
+       ret = remap_pfn_range(vma, data_addr + VDSO_DATA_SIZE, zero_pfn,
+                             PAGE_SIZE, PAGE_READONLY);
         if (ret)
                 goto out;

If you have any more comments, please let me know, thank you.
I will send v2 after waiting for some more feedbacks.

Thanks,
Tiezhu
  

Hi, Tiezhu,

On Sat, May 20, 2023 at 6:35 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>
>
> On 05/18/2023 10:25 AM, Huacai Chen wrote:
> > Hi, Tiezhu,
> >
> > The layout of vdso data (loongarch_vdso_data):
> >
> >        struct vdso_pcpu_data pdata[NR_CPUS];
> >        struct vdso_data data[CS_BASES];
> >
> > VDSO_DATA_SIZE is the page aligned size of loongarch_vdso_data, and in
> > memory, vdso code is above vdso data.
> >
> > Then, get_vdso_base() returns the start of vdso code, and
> > get_vdso_data() returns the start of vdso data.
> >
> > In your patch, __arch_get_timens_vdso_data() returns get_vdso_data() +
> > PAGE_SIZE, but you don't increase the size of loongarch_vdso_data. The
> > result is it returns an address in vdso code.
> >
> > Now, do you know what the problem is? Or still insist that "I have tested"?
>
> Please review the following changes based on the current patch,
> modify the layout of vvar to expand a page size for timens_data,
> and also map it to zero pfn before creating time namespace, then
> the last thing is to add the callback function vvar_fault().
>
> $ git diff
> diff --git a/arch/loongarch/include/asm/page.h
> b/arch/loongarch/include/asm/page.h
> index fb5338b..26e8dcc 100644
> --- a/arch/loongarch/include/asm/page.h
> +++ b/arch/loongarch/include/asm/page.h
> @@ -81,6 +81,7 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>   #define __va(x)                ((void *)((unsigned long)(x) +
> PAGE_OFFSET - PHYS_OFFSET))
>
>   #define pfn_to_kaddr(pfn)      __va((pfn) << PAGE_SHIFT)
> +#define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
>
>   #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
>   #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
> diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
> index cf62103..3e89aca 100644
> --- a/arch/loongarch/kernel/vdso.c
> +++ b/arch/loongarch/kernel/vdso.c
> @@ -23,7 +23,27 @@
>   #include <vdso/vsyscall.h>
>   #include <generated/vdso-offsets.h>
>
> +/*
> + * The layout of vvar:
> + *
> + *                 high
> + * +----------------+----------------+
> + * | timens_data    | PAGE_SIZE      |
> + * +----------------+----------------+
> + * | vdso_data      |                |
> + * | vdso_pcpu_data | VDSO_DATA_SIZE |
> + * +----------------+----------------+
> + *                 low
> + */
> +#define VVAR_SIZE (VDSO_DATA_SIZE + PAGE_SIZE)
> +
> +enum vvar_pages {
> +       VVAR_DATA_PAGE_OFFSET,
> +       VVAR_TIMENS_PAGE_OFFSET,
> +};
You suppose that vdso_data+vdso_pcpu_data can fit in one page, but
this isn't always the case.

> +
>   extern char vdso_start[], vdso_end[];
> +extern unsigned long zero_pfn;
>
>   /* Kernel-provided data used by the VDSO. */
>   static union {
> @@ -42,6 +62,40 @@ static int vdso_mremap(const struct
> vm_special_mapping *sm, struct vm_area_struc
>          return 0;
>   }
>
> +static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
> +                            struct vm_area_struct *vma, struct vm_fault
> *vmf)
> +{
> +       struct page *timens_page = find_timens_vvar_page(vma);
> +       unsigned long pfn;
> +
> +       switch (vmf->pgoff) {
> +       case VVAR_DATA_PAGE_OFFSET:
> +               if (timens_page)
> +                       pfn = page_to_pfn(timens_page);
> +               else
> +                       pfn = sym_to_pfn(vdso_data);
> +               break;
> +#ifdef CONFIG_TIME_NS
> +       case VVAR_TIMENS_PAGE_OFFSET:
> +               /*
> +                * If a task belongs to a time namespace then a namespace
> +                * specific VVAR is mapped with the
> VVAR_DATA_PAGE_OFFSET and
> +                * the real VVAR page is mapped with the
> VVAR_TIMENS_PAGE_OFFSET
> +                * offset.
> +                * See also the comment near timens_setup_vdso_data().
> +                */
> +               if (!timens_page)
> +                       return VM_FAULT_SIGBUS;
> +               pfn = sym_to_pfn(vdso_data);
> +               break;
> +#endif /* CONFIG_TIME_NS */
> +       default:
> +               return VM_FAULT_SIGBUS;
> +       }
> +
> +       return vmf_insert_pfn(vma, vmf->address, pfn);
> +}
> +
>   struct loongarch_vdso_info vdso_info = {
>          .vdso = vdso_start,
>          .size = PAGE_SIZE,
> @@ -52,6 +106,7 @@ struct loongarch_vdso_info vdso_info = {
>          },
>          .data_mapping = {
>                  .name = "[vvar]",
> +               .fault = vvar_fault,
I prefer pre-allocate than page-fault if possible.

Huacai
>          },
>          .offset_sigreturn = vdso_offset_sigreturn,
>   };
> @@ -120,7 +175,7 @@ static unsigned long vdso_base(void)
>   int arch_setup_additional_pages(struct linux_binprm *bprm, int
> uses_interp)
>   {
>          int ret;
> -       unsigned long vvar_size, size, data_addr, vdso_addr;
> +       unsigned long size, data_addr, vdso_addr;
>          struct mm_struct *mm = current->mm;
>          struct vm_area_struct *vma;
>          struct loongarch_vdso_info *info = current->thread.vdso;
> @@ -132,17 +187,16 @@ int arch_setup_additional_pages(struct
> linux_binprm *bprm, int uses_interp)
>           * Determine total area size. This includes the VDSO data itself
>           * and the data pages.
>           */
> -       vvar_size = VDSO_DATA_SIZE;
> -       size = vvar_size + info->size;
> +       size = VVAR_SIZE + info->size;
>
>          data_addr = get_unmapped_area(NULL, vdso_base(), size, 0, 0);
>          if (IS_ERR_VALUE(data_addr)) {
>                  ret = data_addr;
>                  goto out;
>          }
> -       vdso_addr = data_addr + VDSO_DATA_SIZE;
> +       vdso_addr = data_addr + VVAR_SIZE;
>
> -       vma = _install_special_mapping(mm, data_addr, vvar_size,
> +       vma = _install_special_mapping(mm, data_addr, VVAR_SIZE,
>                                         VM_READ | VM_MAYREAD,
>                                         &info->data_mapping);
>          if (IS_ERR(vma)) {
> @@ -153,7 +207,12 @@ int arch_setup_additional_pages(struct linux_binprm
> *bprm, int uses_interp)
>          /* Map VDSO data page. */
>          ret = remap_pfn_range(vma, data_addr,
>                                virt_to_phys(&loongarch_vdso_data) >>
> PAGE_SHIFT,
> -                             vvar_size, PAGE_READONLY);
> +                             VDSO_DATA_SIZE, PAGE_READONLY);
> +       if (ret)
> +               goto out;
> +
> +       ret = remap_pfn_range(vma, data_addr + VDSO_DATA_SIZE, zero_pfn,
> +                             PAGE_SIZE, PAGE_READONLY);
>          if (ret)
>                  goto out;
>
> If you have any more comments, please let me know, thank you.
> I will send v2 after waiting for some more feedbacks.
>
> Thanks,
> Tiezhu
>