[2/3] sfd: Make sure we're not overwriting existing files

Submitted by Cyrill Gorcunov on April 3, 2019, 1:22 p.m.

Details

Message ID 20190403132202.10701-3-gorcunov@gmail.com
State New
Series "sfd: Move into separate file and raise limits early"
Headers show

Commit Message

Cyrill Gorcunov April 3, 2019, 1:22 p.m.
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/include/servicefd.h | 19 +++++++++++++++++++
 criu/servicefd.c         | 40 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 54 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
index f8c2814092b7..7be472cf43e0 100644
--- a/criu/include/servicefd.h
+++ b/criu/include/servicefd.h
@@ -1,7 +1,13 @@ 
 #ifndef __CR_SERVICE_FD_H__
 #define __CR_SERVICE_FD_H__
 
+#include <stdio.h>
 #include <stdbool.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "criu-log.h"
 
 enum sfd_type {
 	SERVICE_FD_MIN,
@@ -28,6 +34,19 @@  enum sfd_type {
 struct pstree_item;
 extern bool sfds_protected;
 
+
+#define sfd_verify_target(_type, _old_fd, _new_fd)			\
+	({								\
+		int __ret = 0;						\
+		if (fcntl(_new_fd, F_GETFD) != -1 && errno != EBADF) {	\
+			pr_err("%s busy target %d -> %d\n",		\
+			       sfd_type_name(_type), _old_fd, _new_fd);	\
+			__ret = -1;					\
+		}							\
+		__ret;							\
+	})
+
+extern const char *sfd_type_name(enum sfd_type type);
 extern int init_service_fd(void);
 extern int get_service_fd(enum sfd_type type);
 extern bool is_any_service_fd(int fd);
diff --git a/criu/servicefd.c b/criu/servicefd.c
index a9909735af44..d918313a7181 100644
--- a/criu/servicefd.c
+++ b/criu/servicefd.c
@@ -10,8 +10,6 @@ 
 #include "common/compiler.h"
 #include "common/list.h"
 
-#include "criu-log.h"
-
 #include "util.h"
 #include "bitops.h"
 #include "pstree.h"
@@ -40,6 +38,31 @@  static int sfd_arr[SERVICE_FD_MAX];
  */
 bool sfds_protected = false;
 
+const char *sfd_type_name(enum sfd_type type)
+{
+	static const char *names[] = {
+		[SERVICE_FD_MIN]	= __stringify_1(SERVICE_FD_MIN),
+		[LOG_FD_OFF]		= __stringify_1(LOG_FD_OFF),
+		[IMG_FD_OFF]		= __stringify_1(IMG_FD_OFF),
+		[PROC_FD_OFF]		= __stringify_1(PROC_FD_OFF),
+		[PROC_PID_FD_OFF]	= __stringify_1(PROC_PID_FD_OFF),
+		[CR_PROC_FD_OFF]	= __stringify_1(CR_PROC_FD_OFF),
+		[ROOT_FD_OFF]		= __stringify_1(ROOT_FD_OFF),
+		[CGROUP_YARD]		= __stringify_1(CGROUP_YARD),
+		[USERNSD_SK]		= __stringify_1(USERNSD_SK),
+		[NS_FD_OFF]		= __stringify_1(NS_FD_OFF),
+		[TRANSPORT_FD_OFF]	= __stringify_1(TRANSPORT_FD_OFF),
+		[RPC_SK_OFF]		= __stringify_1(RPC_SK_OFF),
+		[FDSTORE_SK_OFF]	= __stringify_1(FDSTORE_SK_OFF),
+		[SERVICE_FD_MAX]	= __stringify_1(SERVICE_FD_MAX),
+	};
+
+	if (type < ARRAY_SIZE(names))
+		return names[type];
+
+	return "UNKNOWN";
+}
+
 int init_service_fd(void)
 {
 	struct rlimit64 rlimit;
@@ -107,7 +130,8 @@  int service_fd_min_fd(struct pstree_item *item)
 
 static void sfds_protection_bug(enum sfd_type type)
 {
-	pr_err("Service fd %u is being modified in protected context\n", type);
+	pr_err("Service fd %s is being modified in protected context\n",
+	       sfd_type_name(type));
 	print_stack_trace(current ? vpid(current) : 0);
 	BUG();
 }
@@ -128,8 +152,12 @@  int install_service_fd(enum sfd_type type, int fd)
 		return fd;
 	}
 
+	if (sfd_verify_target(type, fd, sfd))
+		return -1;
+
 	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
-		pr_perror("Dup %d -> %d failed", fd, sfd);
+		pr_perror("%s dup %d -> %d failed",
+			  sfd_type_name(type), fd, sfd);
 		close(fd);
 		return -1;
 	}
@@ -166,10 +194,12 @@  static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
 	if (old < 0)
 		return;
 
+	sfd_verify_target(type, old, new);
 	ret = dup2(old, new);
 	if (ret == -1) {
 		if (errno != EBADF)
-			pr_perror("Unable to clone %d->%d", old, new);
+			pr_perror("%s unable to clone %d->%d",
+				  sfd_type_name(type), old, new);
 	} else if (!(rsti(me)->clone_flags & CLONE_FILES))
 		close(old);
 }

Comments

Andrei Vagin April 5, 2019, 6:07 a.m.
On Wed, Apr 03, 2019 at 04:22:01PM +0300, Cyrill Gorcunov wrote:
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/include/servicefd.h | 19 +++++++++++++++++++
>  criu/servicefd.c         | 40 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
> index f8c2814092b7..7be472cf43e0 100644
> --- a/criu/include/servicefd.h
> +++ b/criu/include/servicefd.h
> @@ -1,7 +1,13 @@
>  #ifndef __CR_SERVICE_FD_H__
>  #define __CR_SERVICE_FD_H__
>  
> +#include <stdio.h>
>  #include <stdbool.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "criu-log.h"
>  
>  enum sfd_type {
>  	SERVICE_FD_MIN,
> @@ -28,6 +34,19 @@ enum sfd_type {
>  struct pstree_item;
>  extern bool sfds_protected;
>  
> +
> +#define sfd_verify_target(_type, _old_fd, _new_fd)			\
> +	({								\
> +		int __ret = 0;						\
> +		if (fcntl(_new_fd, F_GETFD) != -1 && errno != EBADF) {	\
> +			pr_err("%s busy target %d -> %d\n",		\
> +			       sfd_type_name(_type), _old_fd, _new_fd);	\
> +			__ret = -1;					\
> +		}							\
> +		__ret;							\
> +	})
> +
> +extern const char *sfd_type_name(enum sfd_type type);
>  extern int init_service_fd(void);
>  extern int get_service_fd(enum sfd_type type);
>  extern bool is_any_service_fd(int fd);
> diff --git a/criu/servicefd.c b/criu/servicefd.c
> index a9909735af44..d918313a7181 100644
> --- a/criu/servicefd.c
> +++ b/criu/servicefd.c
> @@ -10,8 +10,6 @@
>  #include "common/compiler.h"
>  #include "common/list.h"
>  
> -#include "criu-log.h"
> -
>  #include "util.h"
>  #include "bitops.h"
>  #include "pstree.h"
> @@ -40,6 +38,31 @@ static int sfd_arr[SERVICE_FD_MAX];
>   */
>  bool sfds_protected = false;
>  
> +const char *sfd_type_name(enum sfd_type type)
> +{
> +	static const char *names[] = {
> +		[SERVICE_FD_MIN]	= __stringify_1(SERVICE_FD_MIN),
> +		[LOG_FD_OFF]		= __stringify_1(LOG_FD_OFF),
> +		[IMG_FD_OFF]		= __stringify_1(IMG_FD_OFF),
> +		[PROC_FD_OFF]		= __stringify_1(PROC_FD_OFF),
> +		[PROC_PID_FD_OFF]	= __stringify_1(PROC_PID_FD_OFF),
> +		[CR_PROC_FD_OFF]	= __stringify_1(CR_PROC_FD_OFF),
> +		[ROOT_FD_OFF]		= __stringify_1(ROOT_FD_OFF),
> +		[CGROUP_YARD]		= __stringify_1(CGROUP_YARD),
> +		[USERNSD_SK]		= __stringify_1(USERNSD_SK),
> +		[NS_FD_OFF]		= __stringify_1(NS_FD_OFF),
> +		[TRANSPORT_FD_OFF]	= __stringify_1(TRANSPORT_FD_OFF),
> +		[RPC_SK_OFF]		= __stringify_1(RPC_SK_OFF),
> +		[FDSTORE_SK_OFF]	= __stringify_1(FDSTORE_SK_OFF),
> +		[SERVICE_FD_MAX]	= __stringify_1(SERVICE_FD_MAX),
> +	};
> +
> +	if (type < ARRAY_SIZE(names))
> +		return names[type];
> +
> +	return "UNKNOWN";
> +}
> +
>  int init_service_fd(void)
>  {
>  	struct rlimit64 rlimit;
> @@ -107,7 +130,8 @@ int service_fd_min_fd(struct pstree_item *item)
>  
>  static void sfds_protection_bug(enum sfd_type type)
>  {
> -	pr_err("Service fd %u is being modified in protected context\n", type);
> +	pr_err("Service fd %s is being modified in protected context\n",
> +	       sfd_type_name(type));
>  	print_stack_trace(current ? vpid(current) : 0);
>  	BUG();
>  }
> @@ -128,8 +152,12 @@ int install_service_fd(enum sfd_type type, int fd)
>  		return fd;
>  	}
>  
> +	if (sfd_verify_target(type, fd, sfd))

sfd_verify_target() has to be called only if we install a service fd
first time.

> +		return -1;
> +
>  	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
> -		pr_perror("Dup %d -> %d failed", fd, sfd);
> +		pr_perror("%s dup %d -> %d failed",
> +			  sfd_type_name(type), fd, sfd);
>  		close(fd);
>  		return -1;
>  	}
> @@ -166,10 +194,12 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
>  	if (old < 0)
>  		return;
>  
> +	sfd_verify_target(type, old, new);
>  	ret = dup2(old, new);
>  	if (ret == -1) {
>  		if (errno != EBADF)
> -			pr_perror("Unable to clone %d->%d", old, new);
> +			pr_perror("%s unable to clone %d->%d",
> +				  sfd_type_name(type), old, new);
>  	} else if (!(rsti(me)->clone_flags & CLONE_FILES))
>  		close(old);
>  }
> -- 
> 2.20.1
>