An ability to manipulate mm_struct fields was introduced in
sake of CRIU in first place. Later we provide more suitable
and safe operation PR_SET_MM_MAP where all fields to be modifed
are passed in one structure which allows us to make more detailed
verification.
Still old interface remains present for compatibility reason
though CRIU itself already switched to PR_SET_MM_MAP on its
own long ago.
Googling didn't reveal some other users of this operation
so I think it should be safe to remove this interface.
v2:
- Improve warning message
- Drop redundant args check
CC: Andrey Vagin <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Michael Kerrisk <[email protected]>
CC: Yang Shi <[email protected]>
CC: Michal Hocko <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
---
kernel/sys.c | 151 -----------------------------------------------------------
1 file changed, 2 insertions(+), 149 deletions(-)
Index: linux-ml.git/kernel/sys.c
===================================================================
--- linux-ml.git.orig/kernel/sys.c
+++ linux-ml.git/kernel/sys.c
@@ -2053,163 +2053,16 @@ static int prctl_set_mm_map(int opt, con
}
#endif /* CONFIG_CHECKPOINT_RESTORE */
-static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
- unsigned long len)
-{
- /*
- * This doesn't move the auxiliary vector itself since it's pinned to
- * mm_struct, but it permits filling the vector with new values. It's
- * up to the caller to provide sane values here, otherwise userspace
- * tools which use this vector might be unhappy.
- */
- unsigned long user_auxv[AT_VECTOR_SIZE];
-
- if (len > sizeof(user_auxv))
- return -EINVAL;
-
- if (copy_from_user(user_auxv, (const void __user *)addr, len))
- return -EFAULT;
-
- /* Make sure the last entry is always AT_NULL */
- user_auxv[AT_VECTOR_SIZE - 2] = 0;
- user_auxv[AT_VECTOR_SIZE - 1] = 0;
-
- BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
-
- task_lock(current);
- memcpy(mm->saved_auxv, user_auxv, len);
- task_unlock(current);
-
- return 0;
-}
-
static int prctl_set_mm(int opt, unsigned long addr,
unsigned long arg4, unsigned long arg5)
{
- struct mm_struct *mm = current->mm;
- struct prctl_mm_map prctl_map;
- struct vm_area_struct *vma;
- int error;
-
- if (arg5 || (arg4 && (opt != PR_SET_MM_AUXV &&
- opt != PR_SET_MM_MAP &&
- opt != PR_SET_MM_MAP_SIZE)))
- return -EINVAL;
-
#ifdef CONFIG_CHECKPOINT_RESTORE
if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
#endif
- if (!capable(CAP_SYS_RESOURCE))
- return -EPERM;
-
- if (opt == PR_SET_MM_EXE_FILE)
- return prctl_set_mm_exe_file(mm, (unsigned int)addr);
-
- if (opt == PR_SET_MM_AUXV)
- return prctl_set_auxv(mm, addr, arg4);
-
- if (addr >= TASK_SIZE || addr < mmap_min_addr)
- return -EINVAL;
-
- error = -EINVAL;
-
- down_write(&mm->mmap_sem);
- vma = find_vma(mm, addr);
-
- prctl_map.start_code = mm->start_code;
- prctl_map.end_code = mm->end_code;
- prctl_map.start_data = mm->start_data;
- prctl_map.end_data = mm->end_data;
- prctl_map.start_brk = mm->start_brk;
- prctl_map.brk = mm->brk;
- prctl_map.start_stack = mm->start_stack;
- prctl_map.arg_start = mm->arg_start;
- prctl_map.arg_end = mm->arg_end;
- prctl_map.env_start = mm->env_start;
- prctl_map.env_end = mm->env_end;
- prctl_map.auxv = NULL;
- prctl_map.auxv_size = 0;
- prctl_map.exe_fd = -1;
-
- switch (opt) {
- case PR_SET_MM_START_CODE:
- prctl_map.start_code = addr;
- break;
- case PR_SET_MM_END_CODE:
- prctl_map.end_code = addr;
- break;
- case PR_SET_MM_START_DATA:
- prctl_map.start_data = addr;
- break;
- case PR_SET_MM_END_DATA:
- prctl_map.end_data = addr;
- break;
- case PR_SET_MM_START_STACK:
- prctl_map.start_stack = addr;
- break;
- case PR_SET_MM_START_BRK:
- prctl_map.start_brk = addr;
- break;
- case PR_SET_MM_BRK:
- prctl_map.brk = addr;
- break;
- case PR_SET_MM_ARG_START:
- prctl_map.arg_start = addr;
- break;
- case PR_SET_MM_ARG_END:
- prctl_map.arg_end = addr;
- break;
- case PR_SET_MM_ENV_START:
- prctl_map.env_start = addr;
- break;
- case PR_SET_MM_ENV_END:
- prctl_map.env_end = addr;
- break;
- default:
- goto out;
- }
-
- error = validate_prctl_map(&prctl_map);
- if (error)
- goto out;
-
- switch (opt) {
- /*
- * If command line arguments and environment
- * are placed somewhere else on stack, we can
- * set them up here, ARG_START/END to setup
- * command line argumets and ENV_START/END
- * for environment.
- */
- case PR_SET_MM_START_STACK:
- case PR_SET_MM_ARG_START:
- case PR_SET_MM_ARG_END:
- case PR_SET_MM_ENV_START:
- case PR_SET_MM_ENV_END:
- if (!vma) {
- error = -EFAULT;
- goto out;
- }
- }
-
- mm->start_code = prctl_map.start_code;
- mm->end_code = prctl_map.end_code;
- mm->start_data = prctl_map.start_data;
- mm->end_data = prctl_map.end_data;
- mm->start_brk = prctl_map.start_brk;
- mm->brk = prctl_map.brk;
- mm->start_stack = prctl_map.start_stack;
- mm->arg_start = prctl_map.arg_start;
- mm->arg_end = prctl_map.arg_end;
- mm->env_start = prctl_map.env_start;
- mm->env_end = prctl_map.env_end;
-
- error = 0;
-out:
- up_write(&mm->mmap_sem);
- return error;
+ pr_warn_once("PR_SET_MM_* has been removed. Use PR_SET_MM_MAP instead\n");
+ return -EINVAL;
}
#ifdef CONFIG_CHECKPOINT_RESTORE
On Thu 05-04-18 21:26:51, Cyrill Gorcunov wrote:
> An ability to manipulate mm_struct fields was introduced in
> sake of CRIU in first place. Later we provide more suitable
> and safe operation PR_SET_MM_MAP where all fields to be modifed
> are passed in one structure which allows us to make more detailed
> verification.
I hope this will serve as a memento for future single-user APIs
proposals. The whole thing was a bad idea since the beginning.
> Still old interface remains present for compatibility reason
> though CRIU itself already switched to PR_SET_MM_MAP on its
> own long ago.
>
> Googling didn't reveal some other users of this operation
> so I think it should be safe to remove this interface.
>
> v2:
> - Improve warning message
> - Drop redundant args check
>
> CC: Andrey Vagin <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: Pavel Emelyanov <[email protected]>
> CC: Michael Kerrisk <[email protected]>
> CC: Yang Shi <[email protected]>
> CC: Michal Hocko <[email protected]>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
Acked-by: Michal Hocko <[email protected]>
and fingers crossed that we haven't grown other users outside of CRIU
which is quite bound to specific kernels AFAIK.
> ---
> kernel/sys.c | 151 -----------------------------------------------------------
> 1 file changed, 2 insertions(+), 149 deletions(-)
>
> Index: linux-ml.git/kernel/sys.c
> ===================================================================
> --- linux-ml.git.orig/kernel/sys.c
> +++ linux-ml.git/kernel/sys.c
> @@ -2053,163 +2053,16 @@ static int prctl_set_mm_map(int opt, con
> }
> #endif /* CONFIG_CHECKPOINT_RESTORE */
>
> -static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
> - unsigned long len)
> -{
> - /*
> - * This doesn't move the auxiliary vector itself since it's pinned to
> - * mm_struct, but it permits filling the vector with new values. It's
> - * up to the caller to provide sane values here, otherwise userspace
> - * tools which use this vector might be unhappy.
> - */
> - unsigned long user_auxv[AT_VECTOR_SIZE];
> -
> - if (len > sizeof(user_auxv))
> - return -EINVAL;
> -
> - if (copy_from_user(user_auxv, (const void __user *)addr, len))
> - return -EFAULT;
> -
> - /* Make sure the last entry is always AT_NULL */
> - user_auxv[AT_VECTOR_SIZE - 2] = 0;
> - user_auxv[AT_VECTOR_SIZE - 1] = 0;
> -
> - BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
> -
> - task_lock(current);
> - memcpy(mm->saved_auxv, user_auxv, len);
> - task_unlock(current);
> -
> - return 0;
> -}
> -
> static int prctl_set_mm(int opt, unsigned long addr,
> unsigned long arg4, unsigned long arg5)
> {
> - struct mm_struct *mm = current->mm;
> - struct prctl_mm_map prctl_map;
> - struct vm_area_struct *vma;
> - int error;
> -
> - if (arg5 || (arg4 && (opt != PR_SET_MM_AUXV &&
> - opt != PR_SET_MM_MAP &&
> - opt != PR_SET_MM_MAP_SIZE)))
> - return -EINVAL;
> -
> #ifdef CONFIG_CHECKPOINT_RESTORE
> if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
> return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
> #endif
>
> - if (!capable(CAP_SYS_RESOURCE))
> - return -EPERM;
> -
> - if (opt == PR_SET_MM_EXE_FILE)
> - return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> -
> - if (opt == PR_SET_MM_AUXV)
> - return prctl_set_auxv(mm, addr, arg4);
> -
> - if (addr >= TASK_SIZE || addr < mmap_min_addr)
> - return -EINVAL;
> -
> - error = -EINVAL;
> -
> - down_write(&mm->mmap_sem);
> - vma = find_vma(mm, addr);
> -
> - prctl_map.start_code = mm->start_code;
> - prctl_map.end_code = mm->end_code;
> - prctl_map.start_data = mm->start_data;
> - prctl_map.end_data = mm->end_data;
> - prctl_map.start_brk = mm->start_brk;
> - prctl_map.brk = mm->brk;
> - prctl_map.start_stack = mm->start_stack;
> - prctl_map.arg_start = mm->arg_start;
> - prctl_map.arg_end = mm->arg_end;
> - prctl_map.env_start = mm->env_start;
> - prctl_map.env_end = mm->env_end;
> - prctl_map.auxv = NULL;
> - prctl_map.auxv_size = 0;
> - prctl_map.exe_fd = -1;
> -
> - switch (opt) {
> - case PR_SET_MM_START_CODE:
> - prctl_map.start_code = addr;
> - break;
> - case PR_SET_MM_END_CODE:
> - prctl_map.end_code = addr;
> - break;
> - case PR_SET_MM_START_DATA:
> - prctl_map.start_data = addr;
> - break;
> - case PR_SET_MM_END_DATA:
> - prctl_map.end_data = addr;
> - break;
> - case PR_SET_MM_START_STACK:
> - prctl_map.start_stack = addr;
> - break;
> - case PR_SET_MM_START_BRK:
> - prctl_map.start_brk = addr;
> - break;
> - case PR_SET_MM_BRK:
> - prctl_map.brk = addr;
> - break;
> - case PR_SET_MM_ARG_START:
> - prctl_map.arg_start = addr;
> - break;
> - case PR_SET_MM_ARG_END:
> - prctl_map.arg_end = addr;
> - break;
> - case PR_SET_MM_ENV_START:
> - prctl_map.env_start = addr;
> - break;
> - case PR_SET_MM_ENV_END:
> - prctl_map.env_end = addr;
> - break;
> - default:
> - goto out;
> - }
> -
> - error = validate_prctl_map(&prctl_map);
> - if (error)
> - goto out;
> -
> - switch (opt) {
> - /*
> - * If command line arguments and environment
> - * are placed somewhere else on stack, we can
> - * set them up here, ARG_START/END to setup
> - * command line argumets and ENV_START/END
> - * for environment.
> - */
> - case PR_SET_MM_START_STACK:
> - case PR_SET_MM_ARG_START:
> - case PR_SET_MM_ARG_END:
> - case PR_SET_MM_ENV_START:
> - case PR_SET_MM_ENV_END:
> - if (!vma) {
> - error = -EFAULT;
> - goto out;
> - }
> - }
> -
> - mm->start_code = prctl_map.start_code;
> - mm->end_code = prctl_map.end_code;
> - mm->start_data = prctl_map.start_data;
> - mm->end_data = prctl_map.end_data;
> - mm->start_brk = prctl_map.start_brk;
> - mm->brk = prctl_map.brk;
> - mm->start_stack = prctl_map.start_stack;
> - mm->arg_start = prctl_map.arg_start;
> - mm->arg_end = prctl_map.arg_end;
> - mm->env_start = prctl_map.env_start;
> - mm->env_end = prctl_map.env_end;
> -
> - error = 0;
> -out:
> - up_write(&mm->mmap_sem);
> - return error;
> + pr_warn_once("PR_SET_MM_* has been removed. Use PR_SET_MM_MAP instead\n");
> + return -EINVAL;
> }
>
> #ifdef CONFIG_CHECKPOINT_RESTORE
--
Michal Hocko
SUSE Labs
On 4/5/18 11:26 AM, Cyrill Gorcunov wrote:
> An ability to manipulate mm_struct fields was introduced in
> sake of CRIU in first place. Later we provide more suitable
> and safe operation PR_SET_MM_MAP where all fields to be modifed
> are passed in one structure which allows us to make more detailed
> verification.
>
> Still old interface remains present for compatibility reason
> though CRIU itself already switched to PR_SET_MM_MAP on its
> own long ago.
>
> Googling didn't reveal some other users of this operation
> so I think it should be safe to remove this interface.
>
> v2:
> - Improve warning message
> - Drop redundant args check
>
> CC: Andrey Vagin <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: Pavel Emelyanov <[email protected]>
> CC: Michael Kerrisk <[email protected]>
> CC: Yang Shi <[email protected]>
> CC: Michal Hocko <[email protected]>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
Acked-by: Yang Shi <[email protected]>
> ---
> kernel/sys.c | 151 -----------------------------------------------------------
> 1 file changed, 2 insertions(+), 149 deletions(-)
>
> Index: linux-ml.git/kernel/sys.c
> ===================================================================
> --- linux-ml.git.orig/kernel/sys.c
> +++ linux-ml.git/kernel/sys.c
> @@ -2053,163 +2053,16 @@ static int prctl_set_mm_map(int opt, con
> }
> #endif /* CONFIG_CHECKPOINT_RESTORE */
>
> -static int prctl_set_auxv(struct mm_struct *mm, unsigned long addr,
> - unsigned long len)
> -{
> - /*
> - * This doesn't move the auxiliary vector itself since it's pinned to
> - * mm_struct, but it permits filling the vector with new values. It's
> - * up to the caller to provide sane values here, otherwise userspace
> - * tools which use this vector might be unhappy.
> - */
> - unsigned long user_auxv[AT_VECTOR_SIZE];
> -
> - if (len > sizeof(user_auxv))
> - return -EINVAL;
> -
> - if (copy_from_user(user_auxv, (const void __user *)addr, len))
> - return -EFAULT;
> -
> - /* Make sure the last entry is always AT_NULL */
> - user_auxv[AT_VECTOR_SIZE - 2] = 0;
> - user_auxv[AT_VECTOR_SIZE - 1] = 0;
> -
> - BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
> -
> - task_lock(current);
> - memcpy(mm->saved_auxv, user_auxv, len);
> - task_unlock(current);
> -
> - return 0;
> -}
> -
> static int prctl_set_mm(int opt, unsigned long addr,
> unsigned long arg4, unsigned long arg5)
> {
> - struct mm_struct *mm = current->mm;
> - struct prctl_mm_map prctl_map;
> - struct vm_area_struct *vma;
> - int error;
> -
> - if (arg5 || (arg4 && (opt != PR_SET_MM_AUXV &&
> - opt != PR_SET_MM_MAP &&
> - opt != PR_SET_MM_MAP_SIZE)))
> - return -EINVAL;
> -
> #ifdef CONFIG_CHECKPOINT_RESTORE
> if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
> return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
> #endif
>
> - if (!capable(CAP_SYS_RESOURCE))
> - return -EPERM;
> -
> - if (opt == PR_SET_MM_EXE_FILE)
> - return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> -
> - if (opt == PR_SET_MM_AUXV)
> - return prctl_set_auxv(mm, addr, arg4);
> -
> - if (addr >= TASK_SIZE || addr < mmap_min_addr)
> - return -EINVAL;
> -
> - error = -EINVAL;
> -
> - down_write(&mm->mmap_sem);
> - vma = find_vma(mm, addr);
> -
> - prctl_map.start_code = mm->start_code;
> - prctl_map.end_code = mm->end_code;
> - prctl_map.start_data = mm->start_data;
> - prctl_map.end_data = mm->end_data;
> - prctl_map.start_brk = mm->start_brk;
> - prctl_map.brk = mm->brk;
> - prctl_map.start_stack = mm->start_stack;
> - prctl_map.arg_start = mm->arg_start;
> - prctl_map.arg_end = mm->arg_end;
> - prctl_map.env_start = mm->env_start;
> - prctl_map.env_end = mm->env_end;
> - prctl_map.auxv = NULL;
> - prctl_map.auxv_size = 0;
> - prctl_map.exe_fd = -1;
> -
> - switch (opt) {
> - case PR_SET_MM_START_CODE:
> - prctl_map.start_code = addr;
> - break;
> - case PR_SET_MM_END_CODE:
> - prctl_map.end_code = addr;
> - break;
> - case PR_SET_MM_START_DATA:
> - prctl_map.start_data = addr;
> - break;
> - case PR_SET_MM_END_DATA:
> - prctl_map.end_data = addr;
> - break;
> - case PR_SET_MM_START_STACK:
> - prctl_map.start_stack = addr;
> - break;
> - case PR_SET_MM_START_BRK:
> - prctl_map.start_brk = addr;
> - break;
> - case PR_SET_MM_BRK:
> - prctl_map.brk = addr;
> - break;
> - case PR_SET_MM_ARG_START:
> - prctl_map.arg_start = addr;
> - break;
> - case PR_SET_MM_ARG_END:
> - prctl_map.arg_end = addr;
> - break;
> - case PR_SET_MM_ENV_START:
> - prctl_map.env_start = addr;
> - break;
> - case PR_SET_MM_ENV_END:
> - prctl_map.env_end = addr;
> - break;
> - default:
> - goto out;
> - }
> -
> - error = validate_prctl_map(&prctl_map);
> - if (error)
> - goto out;
> -
> - switch (opt) {
> - /*
> - * If command line arguments and environment
> - * are placed somewhere else on stack, we can
> - * set them up here, ARG_START/END to setup
> - * command line argumets and ENV_START/END
> - * for environment.
> - */
> - case PR_SET_MM_START_STACK:
> - case PR_SET_MM_ARG_START:
> - case PR_SET_MM_ARG_END:
> - case PR_SET_MM_ENV_START:
> - case PR_SET_MM_ENV_END:
> - if (!vma) {
> - error = -EFAULT;
> - goto out;
> - }
> - }
> -
> - mm->start_code = prctl_map.start_code;
> - mm->end_code = prctl_map.end_code;
> - mm->start_data = prctl_map.start_data;
> - mm->end_data = prctl_map.end_data;
> - mm->start_brk = prctl_map.start_brk;
> - mm->brk = prctl_map.brk;
> - mm->start_stack = prctl_map.start_stack;
> - mm->arg_start = prctl_map.arg_start;
> - mm->arg_end = prctl_map.arg_end;
> - mm->env_start = prctl_map.env_start;
> - mm->env_end = prctl_map.env_end;
> -
> - error = 0;
> -out:
> - up_write(&mm->mmap_sem);
> - return error;
> + pr_warn_once("PR_SET_MM_* has been removed. Use PR_SET_MM_MAP instead\n");
> + return -EINVAL;
> }
>
> #ifdef CONFIG_CHECKPOINT_RESTORE
On Thu, Apr 05, 2018 at 08:56:50PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 21:26:51, Cyrill Gorcunov wrote:
> > An ability to manipulate mm_struct fields was introduced in
> > sake of CRIU in first place. Later we provide more suitable
> > and safe operation PR_SET_MM_MAP where all fields to be modifed
> > are passed in one structure which allows us to make more detailed
> > verification.
>
> I hope this will serve as a memento for future single-user APIs
> proposals. The whole thing was a bad idea since the beginning.
It wasn't a bad idea at all, kernel evolves with time and the
first version of the api served as it should (actually a number
of things get involved like user-namespaces, mm reworks and
such things). So no, it wasn't bad. Rather to make it more
suitable we provided a second version of the api.
> > Still old interface remains present for compatibility reason
> > though CRIU itself already switched to PR_SET_MM_MAP on its
> > own long ago.
> >
> > Googling didn't reveal some other users of this operation
> > so I think it should be safe to remove this interface.
> >
> > v2:
> > - Improve warning message
> > - Drop redundant args check
> >
> > CC: Andrey Vagin <[email protected]>
> > CC: Andrew Morton <[email protected]>
> > CC: Pavel Emelyanov <[email protected]>
> > CC: Michael Kerrisk <[email protected]>
> > CC: Yang Shi <[email protected]>
> > CC: Michal Hocko <[email protected]>
> > Signed-off-by: Cyrill Gorcunov <[email protected]>
>
> Acked-by: Michal Hocko <[email protected]>
>
> and fingers crossed that we haven't grown other users outside of CRIU
> which is quite bound to specific kernels AFAIK.
Surely.
On Thu, 5 Apr 2018 21:26:51 +0300 Cyrill Gorcunov <[email protected]> wrote:
> Subject: [PATCH v2] prctl: Deprecate non PR_SET_MM_MAP operations
s/Deprecate/remove/ !
> An ability to manipulate mm_struct fields was introduced in
> sake of CRIU in first place. Later we provide more suitable
> and safe operation PR_SET_MM_MAP where all fields to be modifed
> are passed in one structure which allows us to make more detailed
> verification.
>
> Still old interface remains present for compatibility reason
> though CRIU itself already switched to PR_SET_MM_MAP on its
> own long ago.
>
> Googling didn't reveal some other users of this operation
> so I think it should be safe to remove this interface.
It does seem a bit sudden. We could just add a "this interface is
scheduled for removal" warning, then delete the code for real later in
the year. To give people time to migrate or to complain to us.
On Wed, Apr 18, 2018 at 03:28:20PM -0700, Andrew Morton wrote:
> On Thu, 5 Apr 2018 21:26:51 +0300 Cyrill Gorcunov <[email protected]> wrote:
>
> > Subject: [PATCH v2] prctl: Deprecate non PR_SET_MM_MAP operations
>
> s/Deprecate/remove/ !
Thanks!
> >
> > Googling didn't reveal some other users of this operation
> > so I think it should be safe to remove this interface.
>
> It does seem a bit sudden. We could just add a "this interface is
> scheduled for removal" warning, then delete the code for real later in
> the year. To give people time to migrate or to complain to us.
Yes, I remember this rule of deprecation but taking into account
how special this code is (and I spent significant amount of time
trying to find a real example of its usage outside of criu)
I think it should be safe to simply remove it. Surely I won't
ever do this with some common syscall.
On (04/05/18 21:26), Cyrill Gorcunov wrote:
[..]
> -
> #ifdef CONFIG_CHECKPOINT_RESTORE
> if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
> return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
> #endif
>
> - if (!capable(CAP_SYS_RESOURCE))
> - return -EPERM;
> -
> - if (opt == PR_SET_MM_EXE_FILE)
> - return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> -
> - if (opt == PR_SET_MM_AUXV)
> - return prctl_set_auxv(mm, addr, arg4);
Then validate_prctl_map() and prctl_set_mm_exe_file() can be moved
under CONFIG_CHECKPOINT_RESTORE ifdef.
---
kernel/sys.c | 126 +++++++++++++++++++++++++++++------------------------------
1 file changed, 63 insertions(+), 63 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 6bdffe264303..86e5ef1a5612 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1815,68 +1815,7 @@ SYSCALL_DEFINE1(umask, int, mask)
return mask;
}
-static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
-{
- struct fd exe;
- struct file *old_exe, *exe_file;
- struct inode *inode;
- int err;
-
- exe = fdget(fd);
- if (!exe.file)
- return -EBADF;
-
- inode = file_inode(exe.file);
-
- /*
- * Because the original mm->exe_file points to executable file, make
- * sure that this one is executable as well, to avoid breaking an
- * overall picture.
- */
- err = -EACCES;
- if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
- goto exit;
-
- err = inode_permission(inode, MAY_EXEC);
- if (err)
- goto exit;
-
- /*
- * Forbid mm->exe_file change if old file still mapped.
- */
- exe_file = get_mm_exe_file(mm);
- err = -EBUSY;
- if (exe_file) {
- struct vm_area_struct *vma;
-
- down_read(&mm->mmap_sem);
- for (vma = mm->mmap; vma; vma = vma->vm_next) {
- if (!vma->vm_file)
- continue;
- if (path_equal(&vma->vm_file->f_path,
- &exe_file->f_path))
- goto exit_err;
- }
-
- up_read(&mm->mmap_sem);
- fput(exe_file);
- }
-
- err = 0;
- /* set the new file, lockless */
- get_file(exe.file);
- old_exe = xchg(&mm->exe_file, exe.file);
- if (old_exe)
- fput(old_exe);
-exit:
- fdput(exe);
- return err;
-exit_err:
- up_read(&mm->mmap_sem);
- fput(exe_file);
- goto exit;
-}
-
+#ifdef CONFIG_CHECKPOINT_RESTORE
/*
* WARNING: we don't require any capability here so be very careful
* in what is allowed for modification from userspace.
@@ -1968,7 +1907,68 @@ static int validate_prctl_map(struct prctl_mm_map *prctl_map)
return error;
}
-#ifdef CONFIG_CHECKPOINT_RESTORE
+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+ struct fd exe;
+ struct file *old_exe, *exe_file;
+ struct inode *inode;
+ int err;
+
+ exe = fdget(fd);
+ if (!exe.file)
+ return -EBADF;
+
+ inode = file_inode(exe.file);
+
+ /*
+ * Because the original mm->exe_file points to executable file, make
+ * sure that this one is executable as well, to avoid breaking an
+ * overall picture.
+ */
+ err = -EACCES;
+ if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
+ goto exit;
+
+ err = inode_permission(inode, MAY_EXEC);
+ if (err)
+ goto exit;
+
+ /*
+ * Forbid mm->exe_file change if old file still mapped.
+ */
+ exe_file = get_mm_exe_file(mm);
+ err = -EBUSY;
+ if (exe_file) {
+ struct vm_area_struct *vma;
+
+ down_read(&mm->mmap_sem);
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (!vma->vm_file)
+ continue;
+ if (path_equal(&vma->vm_file->f_path,
+ &exe_file->f_path))
+ goto exit_err;
+ }
+
+ up_read(&mm->mmap_sem);
+ fput(exe_file);
+ }
+
+ err = 0;
+ /* set the new file, lockless */
+ get_file(exe.file);
+ old_exe = xchg(&mm->exe_file, exe.file);
+ if (old_exe)
+ fput(old_exe);
+exit:
+ fdput(exe);
+ return err;
+exit_err:
+ up_read(&mm->mmap_sem);
+ fput(exe_file);
+ goto exit;
+}
+
static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data_size)
{
struct prctl_mm_map prctl_map = { .exe_fd = (u32)-1, };
On Fri, Apr 20, 2018 at 11:38:09AM +0900, Sergey Senozhatsky wrote:
> On (04/05/18 21:26), Cyrill Gorcunov wrote:
> [..]
> > -
> > #ifdef CONFIG_CHECKPOINT_RESTORE
> > if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
> > return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
> > #endif
> >
> > - if (!capable(CAP_SYS_RESOURCE))
> > - return -EPERM;
> > -
> > - if (opt == PR_SET_MM_EXE_FILE)
> > - return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> > -
> > - if (opt == PR_SET_MM_AUXV)
> > - return prctl_set_auxv(mm, addr, arg4);
>
> Then validate_prctl_map() and prctl_set_mm_exe_file() can be moved
> under CONFIG_CHECKPOINT_RESTORE ifdef.
I don't mind. Could you please make the patch on top of linux-next?
On (04/20/18 10:02), Cyrill Gorcunov wrote:
> On Fri, Apr 20, 2018 at 11:38:09AM +0900, Sergey Senozhatsky wrote:
> > On (04/05/18 21:26), Cyrill Gorcunov wrote:
> > [..]
> > > -
> > > #ifdef CONFIG_CHECKPOINT_RESTORE
> > > if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE)
> > > return prctl_set_mm_map(opt, (const void __user *)addr, arg4);
> > > #endif
> > >
> > > - if (!capable(CAP_SYS_RESOURCE))
> > > - return -EPERM;
> > > -
> > > - if (opt == PR_SET_MM_EXE_FILE)
> > > - return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> > > -
> > > - if (opt == PR_SET_MM_AUXV)
> > > - return prctl_set_auxv(mm, addr, arg4);
> >
> > Then validate_prctl_map() and prctl_set_mm_exe_file() can be moved
> > under CONFIG_CHECKPOINT_RESTORE ifdef.
>
> I don't mind. Could you please make the patch on top of linux-next?
As far as I can see, it's not in linux-next yet. So the following is
against the mmots tree. I wouldn't mind it if we could just squash the
patches.
=======================================================================
From: Sergey Senozhatsky <[email protected]>
Subject: [PATCH] prctl: Don't compile some of prctl functions when CRUI
disabled
CHECKPOINT_RESTORE is the only user of validate_prctl_map()
and prctl_set_mm_exe_file(), so we can move those two under
CONFIG_CHECKPOINT_RESTORE.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
kernel/sys.c | 126 +++++++++++++++++++++++++--------------------------
1 file changed, 63 insertions(+), 63 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 6bdffe264303..86e5ef1a5612 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1815,68 +1815,7 @@ SYSCALL_DEFINE1(umask, int, mask)
return mask;
}
-static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
-{
- struct fd exe;
- struct file *old_exe, *exe_file;
- struct inode *inode;
- int err;
-
- exe = fdget(fd);
- if (!exe.file)
- return -EBADF;
-
- inode = file_inode(exe.file);
-
- /*
- * Because the original mm->exe_file points to executable file, make
- * sure that this one is executable as well, to avoid breaking an
- * overall picture.
- */
- err = -EACCES;
- if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
- goto exit;
-
- err = inode_permission(inode, MAY_EXEC);
- if (err)
- goto exit;
-
- /*
- * Forbid mm->exe_file change if old file still mapped.
- */
- exe_file = get_mm_exe_file(mm);
- err = -EBUSY;
- if (exe_file) {
- struct vm_area_struct *vma;
-
- down_read(&mm->mmap_sem);
- for (vma = mm->mmap; vma; vma = vma->vm_next) {
- if (!vma->vm_file)
- continue;
- if (path_equal(&vma->vm_file->f_path,
- &exe_file->f_path))
- goto exit_err;
- }
-
- up_read(&mm->mmap_sem);
- fput(exe_file);
- }
-
- err = 0;
- /* set the new file, lockless */
- get_file(exe.file);
- old_exe = xchg(&mm->exe_file, exe.file);
- if (old_exe)
- fput(old_exe);
-exit:
- fdput(exe);
- return err;
-exit_err:
- up_read(&mm->mmap_sem);
- fput(exe_file);
- goto exit;
-}
-
+#ifdef CONFIG_CHECKPOINT_RESTORE
/*
* WARNING: we don't require any capability here so be very careful
* in what is allowed for modification from userspace.
@@ -1968,7 +1907,68 @@ static int validate_prctl_map(struct prctl_mm_map *prctl_map)
return error;
}
-#ifdef CONFIG_CHECKPOINT_RESTORE
+static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
+{
+ struct fd exe;
+ struct file *old_exe, *exe_file;
+ struct inode *inode;
+ int err;
+
+ exe = fdget(fd);
+ if (!exe.file)
+ return -EBADF;
+
+ inode = file_inode(exe.file);
+
+ /*
+ * Because the original mm->exe_file points to executable file, make
+ * sure that this one is executable as well, to avoid breaking an
+ * overall picture.
+ */
+ err = -EACCES;
+ if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
+ goto exit;
+
+ err = inode_permission(inode, MAY_EXEC);
+ if (err)
+ goto exit;
+
+ /*
+ * Forbid mm->exe_file change if old file still mapped.
+ */
+ exe_file = get_mm_exe_file(mm);
+ err = -EBUSY;
+ if (exe_file) {
+ struct vm_area_struct *vma;
+
+ down_read(&mm->mmap_sem);
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (!vma->vm_file)
+ continue;
+ if (path_equal(&vma->vm_file->f_path,
+ &exe_file->f_path))
+ goto exit_err;
+ }
+
+ up_read(&mm->mmap_sem);
+ fput(exe_file);
+ }
+
+ err = 0;
+ /* set the new file, lockless */
+ get_file(exe.file);
+ old_exe = xchg(&mm->exe_file, exe.file);
+ if (old_exe)
+ fput(old_exe);
+exit:
+ fdput(exe);
+ return err;
+exit_err:
+ up_read(&mm->mmap_sem);
+ fput(exe_file);
+ goto exit;
+}
+
static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data_size)
{
struct prctl_mm_map prctl_map = { .exe_fd = (u32)-1, };
--
2.17.0
On Fri, Apr 20, 2018 at 04:29:03PM +0900, Sergey Senozhatsky wrote:
> >
> > I don't mind. Could you please make the patch on top of linux-next?
>
> As far as I can see, it's not in linux-next yet. So the following is
> against the mmots tree. I wouldn't mind it if we could just squash the
> patches.
>
> =======================================================================
>
> From: Sergey Senozhatsky <[email protected]>
> Subject: [PATCH] prctl: Don't compile some of prctl functions when CRUI
> disabled
>
> CHECKPOINT_RESTORE is the only user of validate_prctl_map()
> and prctl_set_mm_exe_file(), so we can move those two under
> CONFIG_CHECKPOINT_RESTORE.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
Acked-by: Cyrill Gorcunov <[email protected]>
Thanks a lot, Sergey!
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/prctl-Don-t-compile-some-of-prctl-functions-when-CRUI/20180421-040826
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/sys.c: In function 'prctl_set_mm':
>> kernel/sys.c:2108:10: error: implicit declaration of function 'prctl_set_mm_exe_file'; did you mean 'set_mm_exe_file'? [-Werror=implicit-function-declaration]
return prctl_set_mm_exe_file(mm, (unsigned int)addr);
^~~~~~~~~~~~~~~~~~~~~
set_mm_exe_file
>> kernel/sys.c:2174:10: error: implicit declaration of function 'validate_prctl_map'; did you mean 'validate_creds'? [-Werror=implicit-function-declaration]
error = validate_prctl_map(&prctl_map);
^~~~~~~~~~~~~~~~~~
validate_creds
cc1: some warnings being treated as errors
vim +2108 kernel/sys.c
f606b77f1 Cyrill Gorcunov 2014-10-09 2103
79f0713d4 Cyrill Gorcunov 2012-03-15 2104 if (!capable(CAP_SYS_RESOURCE))
028ee4be3 Cyrill Gorcunov 2012-01-12 2105 return -EPERM;
028ee4be3 Cyrill Gorcunov 2012-01-12 2106
6e399cd14 Davidlohr Bueso 2015-04-16 2107 if (opt == PR_SET_MM_EXE_FILE)
6e399cd14 Davidlohr Bueso 2015-04-16 @2108 return prctl_set_mm_exe_file(mm, (unsigned int)addr);
b32dfe377 Cyrill Gorcunov 2012-05-31 2109
4a00e9df2 Alexey Dobriyan 2015-06-25 2110 if (opt == PR_SET_MM_AUXV)
4a00e9df2 Alexey Dobriyan 2015-06-25 2111 return prctl_set_auxv(mm, addr, arg4);
4a00e9df2 Alexey Dobriyan 2015-06-25 2112
1ad75b9e1 Cyrill Gorcunov 2012-06-07 2113 if (addr >= TASK_SIZE || addr < mmap_min_addr)
028ee4be3 Cyrill Gorcunov 2012-01-12 2114 return -EINVAL;
028ee4be3 Cyrill Gorcunov 2012-01-12 2115
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2116 error = -EINVAL;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2117
ddf1d398e Mateusz Guzik 2016-01-20 2118 down_write(&mm->mmap_sem);
028ee4be3 Cyrill Gorcunov 2012-01-12 2119 vma = find_vma(mm, addr);
028ee4be3 Cyrill Gorcunov 2012-01-12 2120
4a00e9df2 Alexey Dobriyan 2015-06-25 2121 prctl_map.start_code = mm->start_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2122 prctl_map.end_code = mm->end_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2123 prctl_map.start_data = mm->start_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2124 prctl_map.end_data = mm->end_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2125 prctl_map.start_brk = mm->start_brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2126 prctl_map.brk = mm->brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2127 prctl_map.start_stack = mm->start_stack;
4a00e9df2 Alexey Dobriyan 2015-06-25 2128 prctl_map.arg_start = mm->arg_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2129 prctl_map.arg_end = mm->arg_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2130 prctl_map.env_start = mm->env_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2131 prctl_map.env_end = mm->env_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2132 prctl_map.auxv = NULL;
4a00e9df2 Alexey Dobriyan 2015-06-25 2133 prctl_map.auxv_size = 0;
4a00e9df2 Alexey Dobriyan 2015-06-25 2134 prctl_map.exe_fd = -1;
4a00e9df2 Alexey Dobriyan 2015-06-25 2135
028ee4be3 Cyrill Gorcunov 2012-01-12 2136 switch (opt) {
028ee4be3 Cyrill Gorcunov 2012-01-12 2137 case PR_SET_MM_START_CODE:
4a00e9df2 Alexey Dobriyan 2015-06-25 2138 prctl_map.start_code = addr;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2139 break;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2140 case PR_SET_MM_END_CODE:
4a00e9df2 Alexey Dobriyan 2015-06-25 2141 prctl_map.end_code = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2142 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2143 case PR_SET_MM_START_DATA:
4a00e9df2 Alexey Dobriyan 2015-06-25 2144 prctl_map.start_data = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2145 break;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2146 case PR_SET_MM_END_DATA:
4a00e9df2 Alexey Dobriyan 2015-06-25 2147 prctl_map.end_data = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2148 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2149 case PR_SET_MM_START_STACK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2150 prctl_map.start_stack = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2151 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2152 case PR_SET_MM_START_BRK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2153 prctl_map.start_brk = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2154 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2155 case PR_SET_MM_BRK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2156 prctl_map.brk = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2157 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2158 case PR_SET_MM_ARG_START:
4a00e9df2 Alexey Dobriyan 2015-06-25 2159 prctl_map.arg_start = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2160 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2161 case PR_SET_MM_ARG_END:
4a00e9df2 Alexey Dobriyan 2015-06-25 2162 prctl_map.arg_end = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2163 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2164 case PR_SET_MM_ENV_START:
4a00e9df2 Alexey Dobriyan 2015-06-25 2165 prctl_map.env_start = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2166 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2167 case PR_SET_MM_ENV_END:
4a00e9df2 Alexey Dobriyan 2015-06-25 2168 prctl_map.env_end = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2169 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2170 default:
028ee4be3 Cyrill Gorcunov 2012-01-12 2171 goto out;
4a00e9df2 Alexey Dobriyan 2015-06-25 2172 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2173
4a00e9df2 Alexey Dobriyan 2015-06-25 @2174 error = validate_prctl_map(&prctl_map);
4a00e9df2 Alexey Dobriyan 2015-06-25 2175 if (error)
028ee4be3 Cyrill Gorcunov 2012-01-12 2176 goto out;
028ee4be3 Cyrill Gorcunov 2012-01-12 2177
4a00e9df2 Alexey Dobriyan 2015-06-25 2178 switch (opt) {
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2179 /*
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2180 * If command line arguments and environment
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2181 * are placed somewhere else on stack, we can
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2182 * set them up here, ARG_START/END to setup
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2183 * command line argumets and ENV_START/END
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2184 * for environment.
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2185 */
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2186 case PR_SET_MM_START_STACK:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2187 case PR_SET_MM_ARG_START:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2188 case PR_SET_MM_ARG_END:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2189 case PR_SET_MM_ENV_START:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2190 case PR_SET_MM_ENV_END:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2191 if (!vma) {
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2192 error = -EFAULT;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2193 goto out;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2194 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2195 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2196
4a00e9df2 Alexey Dobriyan 2015-06-25 2197 mm->start_code = prctl_map.start_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2198 mm->end_code = prctl_map.end_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2199 mm->start_data = prctl_map.start_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2200 mm->end_data = prctl_map.end_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2201 mm->start_brk = prctl_map.start_brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2202 mm->brk = prctl_map.brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2203 mm->start_stack = prctl_map.start_stack;
4a00e9df2 Alexey Dobriyan 2015-06-25 2204 mm->arg_start = prctl_map.arg_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2205 mm->arg_end = prctl_map.arg_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2206 mm->env_start = prctl_map.env_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2207 mm->env_end = prctl_map.env_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2208
028ee4be3 Cyrill Gorcunov 2012-01-12 2209 error = 0;
028ee4be3 Cyrill Gorcunov 2012-01-12 2210 out:
ddf1d398e Mateusz Guzik 2016-01-20 2211 up_write(&mm->mmap_sem);
028ee4be3 Cyrill Gorcunov 2012-01-12 2212 return error;
028ee4be3 Cyrill Gorcunov 2012-01-12 2213 }
300f786b2 Cyrill Gorcunov 2012-06-07 2214
:::::: The code at line 2108 was first introduced by commit
:::::: 6e399cd144d8500ffb5d40fa6848890e2580a80a prctl: avoid using mmap_sem for exe_file serialization
:::::: TO: Davidlohr Bueso <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, 21 Apr 2018 04:37:41 +0800 kbuild test robot <[email protected]> wrote:
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.17-rc1 next-20180420]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/prctl-Don-t-compile-some-of-prctl-functions-when-CRUI/20180421-040826
> config: i386-tinyconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
> kernel/sys.c: In function 'prctl_set_mm':
> >> kernel/sys.c:2108:10: error: implicit declaration of function 'prctl_set_mm_exe_file'; did you mean 'set_mm_exe_file'? [-Werror=implicit-function-declaration]
> return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> ^~~~~~~~~~~~~~~~~~~~~
"prctl: don't compile some of prctl functions when CRUI disabled" was a
fix against "prctl: remove non PR_SET_MM_MAP operations", and it
appears that the base patch wasn't applied when this test was
performed.
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/prctl-Don-t-compile-some-of-prctl-functions-when-CRUI/20180421-040826
config: i386-randconfig-s1-201815 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/sys.c: In function 'prctl_set_mm':
>> kernel/sys.c:2108:10: error: implicit declaration of function 'prctl_set_mm_exe_file' [-Werror=implicit-function-declaration]
return prctl_set_mm_exe_file(mm, (unsigned int)addr);
^~~~~~~~~~~~~~~~~~~~~
>> kernel/sys.c:2174:10: error: implicit declaration of function 'validate_prctl_map' [-Werror=implicit-function-declaration]
error = validate_prctl_map(&prctl_map);
^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/prctl_set_mm_exe_file +2108 kernel/sys.c
f606b77f1 Cyrill Gorcunov 2014-10-09 2103
79f0713d4 Cyrill Gorcunov 2012-03-15 2104 if (!capable(CAP_SYS_RESOURCE))
028ee4be3 Cyrill Gorcunov 2012-01-12 2105 return -EPERM;
028ee4be3 Cyrill Gorcunov 2012-01-12 2106
6e399cd14 Davidlohr Bueso 2015-04-16 2107 if (opt == PR_SET_MM_EXE_FILE)
6e399cd14 Davidlohr Bueso 2015-04-16 @2108 return prctl_set_mm_exe_file(mm, (unsigned int)addr);
b32dfe377 Cyrill Gorcunov 2012-05-31 2109
4a00e9df2 Alexey Dobriyan 2015-06-25 2110 if (opt == PR_SET_MM_AUXV)
4a00e9df2 Alexey Dobriyan 2015-06-25 2111 return prctl_set_auxv(mm, addr, arg4);
4a00e9df2 Alexey Dobriyan 2015-06-25 2112
1ad75b9e1 Cyrill Gorcunov 2012-06-07 2113 if (addr >= TASK_SIZE || addr < mmap_min_addr)
028ee4be3 Cyrill Gorcunov 2012-01-12 2114 return -EINVAL;
028ee4be3 Cyrill Gorcunov 2012-01-12 2115
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2116 error = -EINVAL;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2117
ddf1d398e Mateusz Guzik 2016-01-20 2118 down_write(&mm->mmap_sem);
028ee4be3 Cyrill Gorcunov 2012-01-12 2119 vma = find_vma(mm, addr);
028ee4be3 Cyrill Gorcunov 2012-01-12 2120
4a00e9df2 Alexey Dobriyan 2015-06-25 2121 prctl_map.start_code = mm->start_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2122 prctl_map.end_code = mm->end_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2123 prctl_map.start_data = mm->start_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2124 prctl_map.end_data = mm->end_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2125 prctl_map.start_brk = mm->start_brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2126 prctl_map.brk = mm->brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2127 prctl_map.start_stack = mm->start_stack;
4a00e9df2 Alexey Dobriyan 2015-06-25 2128 prctl_map.arg_start = mm->arg_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2129 prctl_map.arg_end = mm->arg_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2130 prctl_map.env_start = mm->env_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2131 prctl_map.env_end = mm->env_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2132 prctl_map.auxv = NULL;
4a00e9df2 Alexey Dobriyan 2015-06-25 2133 prctl_map.auxv_size = 0;
4a00e9df2 Alexey Dobriyan 2015-06-25 2134 prctl_map.exe_fd = -1;
4a00e9df2 Alexey Dobriyan 2015-06-25 2135
028ee4be3 Cyrill Gorcunov 2012-01-12 2136 switch (opt) {
028ee4be3 Cyrill Gorcunov 2012-01-12 2137 case PR_SET_MM_START_CODE:
4a00e9df2 Alexey Dobriyan 2015-06-25 2138 prctl_map.start_code = addr;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2139 break;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2140 case PR_SET_MM_END_CODE:
4a00e9df2 Alexey Dobriyan 2015-06-25 2141 prctl_map.end_code = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2142 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2143 case PR_SET_MM_START_DATA:
4a00e9df2 Alexey Dobriyan 2015-06-25 2144 prctl_map.start_data = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2145 break;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2146 case PR_SET_MM_END_DATA:
4a00e9df2 Alexey Dobriyan 2015-06-25 2147 prctl_map.end_data = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2148 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2149 case PR_SET_MM_START_STACK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2150 prctl_map.start_stack = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2151 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2152 case PR_SET_MM_START_BRK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2153 prctl_map.start_brk = addr;
028ee4be3 Cyrill Gorcunov 2012-01-12 2154 break;
028ee4be3 Cyrill Gorcunov 2012-01-12 2155 case PR_SET_MM_BRK:
4a00e9df2 Alexey Dobriyan 2015-06-25 2156 prctl_map.brk = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2157 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2158 case PR_SET_MM_ARG_START:
4a00e9df2 Alexey Dobriyan 2015-06-25 2159 prctl_map.arg_start = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2160 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2161 case PR_SET_MM_ARG_END:
4a00e9df2 Alexey Dobriyan 2015-06-25 2162 prctl_map.arg_end = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2163 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2164 case PR_SET_MM_ENV_START:
4a00e9df2 Alexey Dobriyan 2015-06-25 2165 prctl_map.env_start = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2166 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2167 case PR_SET_MM_ENV_END:
4a00e9df2 Alexey Dobriyan 2015-06-25 2168 prctl_map.env_end = addr;
4a00e9df2 Alexey Dobriyan 2015-06-25 2169 break;
4a00e9df2 Alexey Dobriyan 2015-06-25 2170 default:
028ee4be3 Cyrill Gorcunov 2012-01-12 2171 goto out;
4a00e9df2 Alexey Dobriyan 2015-06-25 2172 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2173
4a00e9df2 Alexey Dobriyan 2015-06-25 @2174 error = validate_prctl_map(&prctl_map);
4a00e9df2 Alexey Dobriyan 2015-06-25 2175 if (error)
028ee4be3 Cyrill Gorcunov 2012-01-12 2176 goto out;
028ee4be3 Cyrill Gorcunov 2012-01-12 2177
4a00e9df2 Alexey Dobriyan 2015-06-25 2178 switch (opt) {
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2179 /*
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2180 * If command line arguments and environment
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2181 * are placed somewhere else on stack, we can
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2182 * set them up here, ARG_START/END to setup
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2183 * command line argumets and ENV_START/END
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2184 * for environment.
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2185 */
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2186 case PR_SET_MM_START_STACK:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2187 case PR_SET_MM_ARG_START:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2188 case PR_SET_MM_ARG_END:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2189 case PR_SET_MM_ENV_START:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2190 case PR_SET_MM_ENV_END:
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2191 if (!vma) {
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2192 error = -EFAULT;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2193 goto out;
fe8c7f5cb Cyrill Gorcunov 2012-05-31 2194 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2195 }
028ee4be3 Cyrill Gorcunov 2012-01-12 2196
4a00e9df2 Alexey Dobriyan 2015-06-25 2197 mm->start_code = prctl_map.start_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2198 mm->end_code = prctl_map.end_code;
4a00e9df2 Alexey Dobriyan 2015-06-25 2199 mm->start_data = prctl_map.start_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2200 mm->end_data = prctl_map.end_data;
4a00e9df2 Alexey Dobriyan 2015-06-25 2201 mm->start_brk = prctl_map.start_brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2202 mm->brk = prctl_map.brk;
4a00e9df2 Alexey Dobriyan 2015-06-25 2203 mm->start_stack = prctl_map.start_stack;
4a00e9df2 Alexey Dobriyan 2015-06-25 2204 mm->arg_start = prctl_map.arg_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2205 mm->arg_end = prctl_map.arg_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2206 mm->env_start = prctl_map.env_start;
4a00e9df2 Alexey Dobriyan 2015-06-25 2207 mm->env_end = prctl_map.env_end;
4a00e9df2 Alexey Dobriyan 2015-06-25 2208
028ee4be3 Cyrill Gorcunov 2012-01-12 2209 error = 0;
028ee4be3 Cyrill Gorcunov 2012-01-12 2210 out:
ddf1d398e Mateusz Guzik 2016-01-20 2211 up_write(&mm->mmap_sem);
028ee4be3 Cyrill Gorcunov 2012-01-12 2212 return error;
028ee4be3 Cyrill Gorcunov 2012-01-12 2213 }
300f786b2 Cyrill Gorcunov 2012-06-07 2214
:::::: The code at line 2108 was first introduced by commit
:::::: 6e399cd144d8500ffb5d40fa6848890e2580a80a prctl: avoid using mmap_sem for exe_file serialization
:::::: TO: Davidlohr Bueso <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi.
I see this discussion somewhat faded away since the previous year.
There was rework [1] that reduced (ab)use of mmap_sem in prctl
functions.
Actually, there still remains the down_write() in prctl_set_mm.
I considered at least replacing it with the mm_struct.arg_lock +
down_read() but then I learnt about this thread intending to remove that
part completely. I wouldn't oppose if CRIU is the sole (aware) user.
Ad the bot build issue, I could build the kernel both with
CONFIG_CHECKPOINT_RESTORE and without CONFIG_CHECKPOINT_RESTORE just
fine after applying the two proposed patches.
What is the current state? Perhaps, this change should be CCed to
[email protected](?).
Thanks,
Michal
[1] https://lore.kernel.org/lkml/[email protected]/T/
On Wed, Apr 17, 2019 at 02:23:50PM +0200, Michal Koutn? wrote:
> Hi.
>
> I see this discussion somewhat faded away since the previous year.
>
> There was rework [1] that reduced (ab)use of mmap_sem in prctl
> functions.
>
> Actually, there still remains the down_write() in prctl_set_mm.
> I considered at least replacing it with the mm_struct.arg_lock +
> down_read() but then I learnt about this thread intending to remove that
> part completely. I wouldn't oppose if CRIU is the sole (aware) user.
>
> Ad the bot build issue, I could build the kernel both with
> CONFIG_CHECKPOINT_RESTORE and without CONFIG_CHECKPOINT_RESTORE just
> fine after applying the two proposed patches.
>
> What is the current state? Perhaps, this change should be CCed to
> [email protected](?).
>
> [1] https://lore.kernel.org/lkml/[email protected]/T/
Hi! I've a bit vague memory what we've ended up with, but iirc there was
a problem with brk() syscall or similar. Then I think we left everything
as is. I think there is no much activity in this prctl area now as far
as i know (replying to what is current state).
On Wed, Apr 17, 2019 at 03:38:41PM +0300, Cyrill Gorcunov <[email protected]> wrote:
> I've a bit vague memory what we've ended up with, but iirc there was
> a problem with brk() syscall or similar. Then I think we left everything
> as is.
Was this related to the removal of non PR_SET_MM_MAP operations too?
Do you have any pointers to the topic?
Thanks,
Michal
On Wed, Apr 17, 2019 at 04:44:54PM +0200, Michal Koutn? wrote:
> On Wed, Apr 17, 2019 at 03:38:41PM +0300, Cyrill Gorcunov <[email protected]> wrote:
> > I've a bit vague memory what we've ended up with, but iirc there was
> > a problem with brk() syscall or similar. Then I think we left everything
> > as is.
>
> Was this related to the removal of non PR_SET_MM_MAP operations too?
> Do you have any pointers to the topic?
Gimme some time, will reply later.
On Wed, Apr 17, 2019 at 04:44:54PM +0200, Michal Koutn? wrote:
> On Wed, Apr 17, 2019 at 03:38:41PM +0300, Cyrill Gorcunov <[email protected]> wrote:
> > I've a bit vague memory what we've ended up with, but iirc there was
> > a problem with brk() syscall or similar. Then I think we left everything
> > as is.
> Was this related to the removal of non PR_SET_MM_MAP operations too?
> Do you have any pointers to the topic?
Here some details: suprisingly there are already users of PR_SET_MM_x
operands, so we can't deprecate it (https://lkml.org/lkml/2018/5/6/127).
And then there was an attempt to change locks https://lore.kernel.org/patchwork/patch/
but since we're modifying mm-> entries we should gurd them against
simultaneous sys_brk call. Or I misunderstood you and you meant somthing
different?