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

Submitted by Cyrill Gorcunov on March 29, 2019, 5:55 p.m.

Details

Message ID 20190329175524.7145-3-gorcunov@gmail.com
State New
Series "sfd: Move service routines into separate file"
Headers show

Commit Message

Cyrill Gorcunov March 29, 2019, 5:55 p.m.
Just a warn for now since we need to investigate
every case without breaking ci.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/include/servicefd.h | 15 +++++++++++++++
 criu/servicefd.c         |  4 ++--
 2 files changed, 17 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
index f8c2814092b7..1c4349e4eeb0 100644
--- a/criu/include/servicefd.h
+++ b/criu/include/servicefd.h
@@ -1,7 +1,11 @@ 
 #ifndef __CR_SERVICE_FD_H__
 #define __CR_SERVICE_FD_H__
 
+#include <stdio.h>
 #include <stdbool.h>
+#include <unistd.h>
+
+#include "criu-log.h"
 
 enum sfd_type {
 	SERVICE_FD_MIN,
@@ -28,6 +32,17 @@  enum sfd_type {
 struct pstree_item;
 extern bool sfds_protected;
 
+#define sfd_verify_targtet(_type, _old_fd, _new_fd)					\
+	do {										\
+		char self_path[64];							\
+											\
+		snprintf(self_path, sizeof(self_path), "/proc/self/fd/%d", _new_fd);	\
+		if (access(self_path, F_OK)) {						\
+			pr_warn("type %d: busy target %d -> %d\n",			\
+				_type, _old_fd, _new_fd);				\
+		}									\
+	} while (0)
+
 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..bb8c26ca466c 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"
@@ -128,6 +126,7 @@  int install_service_fd(enum sfd_type type, int fd)
 		return fd;
 	}
 
+	sfd_verify_targtet(type, fd, sfd);
 	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
 		pr_perror("Dup %d -> %d failed", fd, sfd);
 		close(fd);
@@ -166,6 +165,7 @@  static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
 	if (old < 0)
 		return;
 
+	sfd_verify_targtet(type, old, new);
 	ret = dup2(old, new);
 	if (ret == -1) {
 		if (errno != EBADF)

Comments

Pavel Emelianov April 1, 2019, 3:35 p.m.
On 3/29/19 8:55 PM, Cyrill Gorcunov wrote:
> Just a warn for now since we need to investigate
> every case without breaking ci.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/include/servicefd.h | 15 +++++++++++++++
>  criu/servicefd.c         |  4 ++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
> index f8c2814092b7..1c4349e4eeb0 100644
> --- a/criu/include/servicefd.h
> +++ b/criu/include/servicefd.h
> @@ -1,7 +1,11 @@
>  #ifndef __CR_SERVICE_FD_H__
>  #define __CR_SERVICE_FD_H__
>  
> +#include <stdio.h>
>  #include <stdbool.h>
> +#include <unistd.h>
> +
> +#include "criu-log.h"
>  
>  enum sfd_type {
>  	SERVICE_FD_MIN,
> @@ -28,6 +32,17 @@ enum sfd_type {
>  struct pstree_item;
>  extern bool sfds_protected;
>  
> +#define sfd_verify_targtet(_type, _old_fd, _new_fd)					\
> +	do {										\
> +		char self_path[64];							\
> +											\
> +		snprintf(self_path, sizeof(self_path), "/proc/self/fd/%d", _new_fd);	\
> +		if (access(self_path, F_OK)) {						\

Calling fcntl(fd, F_GETFL) is much faster to check whether the fd in question is busy or not.
We even have(d) a reopen_fd_safe() for it.

-- Pavel

> +			pr_warn("type %d: busy target %d -> %d\n",			\
> +				_type, _old_fd, _new_fd);				\
> +		}									\
> +	} while (0)
> +
>  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..bb8c26ca466c 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"
> @@ -128,6 +126,7 @@ int install_service_fd(enum sfd_type type, int fd)
>  		return fd;
>  	}
>  
> +	sfd_verify_targtet(type, fd, sfd);
>  	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
>  		pr_perror("Dup %d -> %d failed", fd, sfd);
>  		close(fd);
> @@ -166,6 +165,7 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
>  	if (old < 0)
>  		return;
>  
> +	sfd_verify_targtet(type, old, new);
>  	ret = dup2(old, new);
>  	if (ret == -1) {
>  		if (errno != EBADF)
>
Cyrill Gorcunov April 1, 2019, 3:52 p.m.
On Mon, Apr 01, 2019 at 03:35:43PM +0000, Pavel Emelianov wrote:
...
> >  
> > +#define sfd_verify_targtet(_type, _old_fd, _new_fd)				\
> > +	do {										\
> > +		char self_path[64];							\
> > +											\
> > +		snprintf(self_path, sizeof(self_path), "/proc/self/fd/%d", _new_fd);	\
> > +		if (access(self_path, F_OK)) {						\
> 
> Calling fcntl(fd, F_GETFL) is much faster to check whether the fd in question is busy or not.
> We even have(d) a reopen_fd_safe() for it.

fcntl is faster but a way less flexible. i thought of using it at first place
but though desided to switch to 'access' helper so that we might need to use
this macro for testing other pid's fds, not just for self. but I tend
to agree that this looks like overkill. Will switch back to fcntl later.
Andrei Vagin April 2, 2019, 5:28 a.m.
On Fri, Mar 29, 2019 at 08:55:23PM +0300, Cyrill Gorcunov wrote:
> Just a warn for now since we need to investigate
> every case without breaking ci.

We don't have these cases, so it should be an error.

Here is one inline comment

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/include/servicefd.h | 15 +++++++++++++++
>  criu/servicefd.c         |  4 ++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
> index f8c2814092b7..1c4349e4eeb0 100644
> --- a/criu/include/servicefd.h
> +++ b/criu/include/servicefd.h
> @@ -1,7 +1,11 @@
>  #ifndef __CR_SERVICE_FD_H__
>  #define __CR_SERVICE_FD_H__
>  
> +#include <stdio.h>
>  #include <stdbool.h>
> +#include <unistd.h>
> +
> +#include "criu-log.h"
>  
>  enum sfd_type {
>  	SERVICE_FD_MIN,
> @@ -28,6 +32,17 @@ enum sfd_type {
>  struct pstree_item;
>  extern bool sfds_protected;
>  
> +#define sfd_verify_targtet(_type, _old_fd, _new_fd)					\
> +	do {										\
> +		char self_path[64];							\
> +											\
> +		snprintf(self_path, sizeof(self_path), "/proc/self/fd/%d", _new_fd);	\
> +		if (access(self_path, F_OK)) {						\
> +			pr_warn("type %d: busy target %d -> %d\n",			\
> +				_type, _old_fd, _new_fd);				\
> +		}									\
> +	} while (0)
> +
>  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..bb8c26ca466c 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"
> @@ -128,6 +126,7 @@ int install_service_fd(enum sfd_type type, int fd)
>  		return fd;
>  	}
>

You have to check that this service_fd has not been installed yet:
	if (!test_bit(type, sfd_map))

> +	sfd_verify_targtet(type, fd, sfd);
>  	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
>  		pr_perror("Dup %d -> %d failed", fd, sfd);
>  		close(fd);
> @@ -166,6 +165,7 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
>  	if (old < 0)
>  		return;
>  
> +	sfd_verify_targtet(type, old, new);
>  	ret = dup2(old, new);
>  	if (ret == -1) {
>  		if (errno != EBADF)
> -- 
> 2.20.1
>
Cyrill Gorcunov April 2, 2019, 7:17 a.m.
On Mon, Apr 01, 2019 at 10:28:10PM -0700, Andrey Vagin wrote:
> On Fri, Mar 29, 2019 at 08:55:23PM +0300, Cyrill Gorcunov wrote:
> > Just a warn for now since we need to investigate
> > every case without breaking ci.
> 
> We don't have these cases, so it should be an error.

We already have these cases as by sfd design, and if
I trigger an error the ci will be broken.

> 
> Here is one inline comment
...
> You have to check that this service_fd has not been installed yet:
> 	if (!test_bit(type, sfd_map))
> 

For install service it is already there. For move_service_fd
it is not and I'm not sure if it should test for sfd_map

> > +	sfd_verify_targtet(type, fd, sfd);
> >  	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
> >  		pr_perror("Dup %d -> %d failed", fd, sfd);
> >  		close(fd);
> > @@ -166,6 +165,7 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
> >  	if (old < 0)
> >  		return;
> >  
> > +	sfd_verify_targtet(type, old, new);
> >  	ret = dup2(old, new);
> >  	if (ret == -1) {
> >  		if (errno != EBADF)
Cyrill Gorcunov April 3, 2019, 8:24 a.m.
On Tue, Apr 02, 2019 at 10:17:46AM +0300, Cyrill Gorcunov wrote:
> On Mon, Apr 01, 2019 at 10:28:10PM -0700, Andrey Vagin wrote:
> > On Fri, Mar 29, 2019 at 08:55:23PM +0300, Cyrill Gorcunov wrote:
> > > Just a warn for now since we need to investigate
> > > every case without breaking ci.
> > 
> > We don't have these cases, so it should be an error.
> 
> We already have these cases as by sfd design, and if
> I trigger an error the ci will be broken.
> 

Actually, no, we don't have -- I happen to make a typo :)
Will rework the series and resend!