tty: Move tty layer shared init into tty_init_restore

Submitted by Cyrill Gorcunov on June 25, 2019, 12:16 p.m.

Details

Message ID 20190625121626.24799-1-gorcunov@gmail.com
State New
Series "tty: Move tty layer shared init into tty_init_restore"
Headers show

Commit Message

Cyrill Gorcunov June 25, 2019, 12:16 p.m.
Instead of using tty_mutex value in atomic context
(which is wrong, since it is not atomic) better move
tty_mutex allocation into cr_restore_tasks where our
all initializers live. Otherwise weird race effect
might be observed.

Reported-by: Deng Guangxing <dengguangxing@huawei.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/cr-restore.c  |  3 +++
 criu/include/tty.h |  1 +
 criu/tty.c         | 14 +-------------
 3 files changed, 5 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index ecfee1296a33..b184a58a0c86 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -2354,6 +2354,9 @@  int cr_restore_tasks(void)
 	if (vdso_init_restore())
 		goto err;
 
+	if (tty_init_restore())
+		goto err;
+
 	if (opts.cpu_cap & CPU_CAP_IMAGE) {
 		if (cpu_validate_cpuinfo())
 			goto err;
diff --git a/criu/include/tty.h b/criu/include/tty.h
index 95ced83964da..8419593e5ebb 100644
--- a/criu/include/tty.h
+++ b/criu/include/tty.h
@@ -32,6 +32,7 @@  struct mount_info;
 extern int devpts_restore(struct mount_info *pm);
 
 extern int tty_prep_fds(void);
+extern int tty_init_restore(void);
 
 extern int devpts_check_bindmount(struct mount_info *m);
 
diff --git a/criu/tty.c b/criu/tty.c
index 6fe11530a73c..82a5b69f8948 100644
--- a/criu/tty.c
+++ b/criu/tty.c
@@ -349,19 +349,14 @@  static mutex_t *tty_mutex;
 
 static bool tty_is_master(struct tty_info *info);
 
-static int init_tty_mutex(void)
+int tty_init_restore(void)
 {
-	if (tty_mutex)
-		return 0;
-
 	tty_mutex = shmalloc(sizeof(*tty_mutex));
 	if (!tty_mutex) {
 		pr_err("Can't create ptmx index mutex\n");
 		return -1;
 	}
-
 	mutex_init(tty_mutex);
-
 	return 0;
 }
 
@@ -1789,13 +1784,6 @@  static int tty_info_setup(struct tty_info *info)
 
 	add_post_prepare_cb_once(&prep_tty_restore);
 
-	/*
-	 * Call it explicitly. Post-callbacks will be called after
-	 * namespaces preparation, while the latter needs this mutex.
-	 */
-	if (init_tty_mutex())
-		return -1;
-
 	info->fdstore_id = -1;
 	return file_desc_add(&info->d, info->tfe->id, &tty_desc_ops);
 }

Comments

Andrei Vagin July 3, 2019, 5:21 p.m.
On Tue, Jun 25, 2019 at 03:16:26PM +0300, Cyrill Gorcunov wrote:
> Instead of using tty_mutex value in atomic context
> (which is wrong, since it is not atomic) better move
> tty_mutex allocation into cr_restore_tasks where our
> all initializers live. Otherwise weird race effect
> might be observed.
> 
> Reported-by: Deng Guangxing <dengguangxing@huawei.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/cr-restore.c  |  3 +++
>  criu/include/tty.h |  1 +
>  criu/tty.c         | 14 +-------------
>  3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index ecfee1296a33..b184a58a0c86 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -2354,6 +2354,9 @@ int cr_restore_tasks(void)
>  	if (vdso_init_restore())
>  		goto err;
>  
> +	if (tty_init_restore())
> +		goto err;
> +
>  	if (opts.cpu_cap & CPU_CAP_IMAGE) {
>  		if (cpu_validate_cpuinfo())
>  			goto err;
> diff --git a/criu/include/tty.h b/criu/include/tty.h
> index 95ced83964da..8419593e5ebb 100644
> --- a/criu/include/tty.h
> +++ b/criu/include/tty.h
> @@ -32,6 +32,7 @@ struct mount_info;
>  extern int devpts_restore(struct mount_info *pm);
>  
>  extern int tty_prep_fds(void);
> +extern int tty_init_restore(void);
>  
>  extern int devpts_check_bindmount(struct mount_info *m);
>  
> diff --git a/criu/tty.c b/criu/tty.c
> index 6fe11530a73c..82a5b69f8948 100644
> --- a/criu/tty.c
> +++ b/criu/tty.c
> @@ -349,19 +349,14 @@ static mutex_t *tty_mutex;
>  
>  static bool tty_is_master(struct tty_info *info);
>  
> -static int init_tty_mutex(void)
> +int tty_init_restore(void)
>  {
> -	if (tty_mutex)
> -		return 0;
> -
>  	tty_mutex = shmalloc(sizeof(*tty_mutex));
>  	if (!tty_mutex) {
>  		pr_err("Can't create ptmx index mutex\n");
>  		return -1;
>  	}
> -

pls, don't remove this and the next blank lines.

>  	mutex_init(tty_mutex);
> -
>  	return 0;
>  }
>  
> @@ -1789,13 +1784,6 @@ static int tty_info_setup(struct tty_info *info)
>  
>  	add_post_prepare_cb_once(&prep_tty_restore);
>  
> -	/*
> -	 * Call it explicitly. Post-callbacks will be called after
> -	 * namespaces preparation, while the latter needs this mutex.
> -	 */
> -	if (init_tty_mutex())
> -		return -1;
> -
>  	info->fdstore_id = -1;
>  	return file_desc_add(&info->d, info->tfe->id, &tty_desc_ops);
>  }
> -- 
> 2.20.1
>
Andrei Vagin July 3, 2019, 5:24 p.m.
Applied, thanks!

On Tue, Jun 25, 2019 at 03:16:26PM +0300, Cyrill Gorcunov wrote:
> Instead of using tty_mutex value in atomic context
> (which is wrong, since it is not atomic) better move
> tty_mutex allocation into cr_restore_tasks where our
> all initializers live. Otherwise weird race effect
> might be observed.
> 
> Reported-by: Deng Guangxing <dengguangxing@huawei.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/cr-restore.c  |  3 +++
>  criu/include/tty.h |  1 +
>  criu/tty.c         | 14 +-------------
>  3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index ecfee1296a33..b184a58a0c86 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -2354,6 +2354,9 @@ int cr_restore_tasks(void)
>  	if (vdso_init_restore())
>  		goto err;
>  
> +	if (tty_init_restore())
> +		goto err;
> +
>  	if (opts.cpu_cap & CPU_CAP_IMAGE) {
>  		if (cpu_validate_cpuinfo())
>  			goto err;
> diff --git a/criu/include/tty.h b/criu/include/tty.h
> index 95ced83964da..8419593e5ebb 100644
> --- a/criu/include/tty.h
> +++ b/criu/include/tty.h
> @@ -32,6 +32,7 @@ struct mount_info;
>  extern int devpts_restore(struct mount_info *pm);
>  
>  extern int tty_prep_fds(void);
> +extern int tty_init_restore(void);
>  
>  extern int devpts_check_bindmount(struct mount_info *m);
>  
> diff --git a/criu/tty.c b/criu/tty.c
> index 6fe11530a73c..82a5b69f8948 100644
> --- a/criu/tty.c
> +++ b/criu/tty.c
> @@ -349,19 +349,14 @@ static mutex_t *tty_mutex;
>  
>  static bool tty_is_master(struct tty_info *info);
>  
> -static int init_tty_mutex(void)
> +int tty_init_restore(void)
>  {
> -	if (tty_mutex)
> -		return 0;
> -
>  	tty_mutex = shmalloc(sizeof(*tty_mutex));
>  	if (!tty_mutex) {
>  		pr_err("Can't create ptmx index mutex\n");
>  		return -1;
>  	}
> -
>  	mutex_init(tty_mutex);
> -
>  	return 0;
>  }
>  
> @@ -1789,13 +1784,6 @@ static int tty_info_setup(struct tty_info *info)
>  
>  	add_post_prepare_cb_once(&prep_tty_restore);
>  
> -	/*
> -	 * Call it explicitly. Post-callbacks will be called after
> -	 * namespaces preparation, while the latter needs this mutex.
> -	 */
> -	if (init_tty_mutex())
> -		return -1;
> -
>  	info->fdstore_id = -1;
>  	return file_desc_add(&info->d, info->tfe->id, &tty_desc_ops);
>  }
> -- 
> 2.20.1
>