prctl: Deprecate non PR_SET_MM_MAP operations

PATCH In review Cyrill Gorcunov 7 participants 18 messages Apr 5, 2018 – Apr 17, 2019

Share & tools

Cyrill Gorcunov

author

2018-04-05 18:26 UTC #0 lore raw

PATCH v2 1/1 prctl: Deprecate non PR_SET_MM_MAP operations

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

kernel/sys.c +2 ~~-149~~

Index: 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

Acked-by on v2 1/1

2018-04-05 18:56 UTC #1 lore raw

Quoted 5 lines · Cyrill Gorcunov

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.

Quoted 11 lines

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

Quoted 7 lines

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.

Quoted 174 lines

---
 kernel/sys.c |  151 -----------------------------------------------------------
 1 file changed, 2 insertions(+), 149 deletions(-)

Index: linux-ml.git/kernel/sys.c
===================================================================

kernel/sys.c +2 ~~-149~~

@@ -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

Signature · 2 lines

Michal Hocko
SUSE Labs

Cyrill Gorcunov

author

2018-04-05 19:52 UTC #2 lore raw

Quoted 9 lines · Michal Hocko

Quoted 5 lines · Cyrill Gorcunov
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.

Quoted 19 lines

Quoted 11 lines
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
Quoted 7 lines
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]>

Quoted 4 lines

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.

Yang Shi

Acked-by on v2 1/1

2018-04-05 19:00 UTC #3 lore raw

In reply to Cyrill Gorcunov

Quoted 17 lines · Cyrill Gorcunov

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

Quoted 7 lines

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]>

Quoted 174 lines

---
  kernel/sys.c |  151 -----------------------------------------------------------
  1 file changed, 2 insertions(+), 149 deletions(-)

Index: linux-ml.git/kernel/sys.c
===================================================================

kernel/sys.c +2 ~~-149~~

@@ -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

Andrew Morton

2018-04-18 22:28 UTC #4 lore raw

In reply to Cyrill Gorcunov

Quoted 1 line · 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/ !

Quoted 6 lines

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.

Quoted 6 lines

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.

Cyrill Gorcunov

author

2018-04-18 22:42 UTC #5 lore raw

Quoted 5 lines · Andrew Morton

Quoted 1 line · 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!

Quoted 7 lines

Quoted 3 lines
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.

Sergey Senozhatsky

2018-04-20 02:38 UTC #6 lore raw

In reply to Cyrill Gorcunov

On (04/05/18 21:26), Cyrill Gorcunov wrote:
[..]

Quoted 4 lines

(quoted diff) +0 -1

-
 #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);

Review

Quoted 10 lines

(quoted diff) +0 -8

 #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(-)

kernel/sys.c +63 ~~-63~~

index 6bdffe264303..86e5ef1a5612 100644
@@ -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, };

Cyrill Gorcunov

author

2018-04-20 07:03 UTC #7 lore raw

Quoted 8 lines · Sergey Senozhatsky

On (04/05/18 21:26), Cyrill Gorcunov wrote:
[..]

Quoted 6 lines

(quoted diff) +0 -1

-
 #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

Quoted 11 lines

Review
Quoted 8 lines
(quoted diff) +0 -8
-	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?

Sergey Senozhatsky

2018-04-20 07:29 UTC #8 lore raw

Quoted 21 lines · Cyrill Gorcunov

Quoted 8 lines · Sergey Senozhatsky
On (04/05/18 21:26), Cyrill Gorcunov wrote:
[..]
Quoted 6 lines
(quoted diff) +0 -1
-
 #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
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +0 -8
-	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.

Quoted 1 line

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

kernel/sys.c +63 ~~-63~~

index 6bdffe264303..86e5ef1a5612 100644
@@ -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, };

Signature · 1 line

2.17.0

Cyrill Gorcunov

author Acked-by

2018-04-20 08:18 UTC #9 lore raw

Quoted 13 lines · Sergey Senozhatsky

Quoted 2 lines
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

Quoted 5 lines

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!

kbuild test robot

build bot

2018-04-20 20:37 UTC #10 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

In reply to Sergey Senozhatsky

I love your patch! Yet something to improve:

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

Andrew Morton

2018-04-20 21:00 UTC #11 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

Quoted 13 lines · 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 

Quoted 6 lines

All errors (new ones prefixed by >>):
kernel/sys.c: In function 'prctl_set_mm':
Quoted 1 line
Quoted 1 line
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.

Michal Koutný

2019-04-17 12:23 UTC #12 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

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/

Cyrill Gorcunov

author

2019-04-17 12:38 UTC #13 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

Quoted 12 lines · Michal Koutný

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.

Quoted 8 lines

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

Michal Koutný

2019-04-17 14:44 UTC #14 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

Quoted 3 lines · Cyrill Gorcunov

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

Cyrill Gorcunov

author

2019-04-17 14:56 UTC #15 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

Quoted 7 lines · Michal Koutný

Quoted 3 lines · Cyrill Gorcunov
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.

Cyrill Gorcunov

author

2019-04-17 16:56 UTC #16 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

In reply to Michal Koutný

Quoted 6 lines · Michal Koutný

Quoted 3 lines · Cyrill Gorcunov
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?

kbuild test robot

build bot

2018-04-20 21:44 UTC #17 lore raw

Re: [PATCH] prctl: Don't compile some of prctl functions when CRUI

In reply to Sergey Senozhatsky

I love your patch! Yet something to improve:

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