[CRIU] [PATCH] pages: Share page_ids between ns dump-helpers

[Kirill Tkhai]

On Thu, Jun 29, 2017 at 23:09, Dmitry Safonov wrote:
> After the commit 9d2e1dfebedf ("ipc: Keep shmem segments contents into
> pagemap/page images"), SysV shmem pages are stored in the pages-<n>.img
> files.  Where <n> is page_ids number.
> 
> The problem here is that we dump namespaces with a helper after fork().
> This results in non-shared between child(s) and parent-criu counter
> with the result of overwriting pages-*.img files later by helpers/criu.
> This resulted in restore with corrupted shmem segments or
> in the following failures on restore:
> > (04.501019)      1: Error (criu/pagemap.c:265): Can't read mapping page 0: No such file or directory
> > (04.637516)      1: Error (criu/pagemap.c:265): Can't read mapping page 0: No such file or directory
> 
> Allocate page_ids with shmalloc() and make it atomic.
> As we need it in pre-dump, dump, page-server and rpc,
> init it early in the main().
> 
> Cc: Pavel Emelyanov <xemul at virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov at virtuozzo.com>
> ---
>  criu/crtools.c       |  3 +++
>  criu/image.c         | 25 +++++++++++++++++++++----
>  criu/include/image.h |  2 ++
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/criu/crtools.c b/criu/crtools.c
> index b0f7b946cc56..a3dbbcb10668 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -325,6 +325,9 @@ int main(int argc, char *argv[], char *envp[])
>  	if (fault_injection_init())
>  		return 1;
>  
> +	if (images_init())
> +		return 1;
> +
>  	cr_pb_init();
>  	setproctitle_init(argc, argv, envp);
>  
> diff --git a/criu/image.c b/criu/image.c
> index 62e8e7109e65..59b9dad5458d 100644
> --- a/criu/image.c
> +++ b/criu/image.c
> @@ -17,6 +17,7 @@
>  #include "images/inventory.pb-c.h"
>  #include "images/pagemap.pb-c.h"
>  #include "img-remote.h"
> +#include "rst-malloc.h"

^^^

>  
>  bool ns_per_id = false;
>  bool img_common_magic = true;
> @@ -505,7 +506,23 @@ void close_image_dir(void)
>  	close_service_fd(IMG_FD_OFF);
>  }
>  
> -static unsigned long page_ids = 1;
> +/*
> + * As we dump SysV shmem IPC into pages-<page_ids>.img,
> + * the keys should be shared in each dump ns-helpers and in criu.
> + */
> +static atomic_t *page_ids;
> +
> +int images_init(void)
> +{
> +	page_ids = shmalloc(sizeof(*page_ids));

I think it's not good to use shmalloc() "as is", because
currently it's aimed for restore code only. All variables,
function and file names are called after restore, and now
you are the first, who begins to use it for dumping.

If one day someone reworks shmalloc() code to fit some
restore goals more, it will be an unpleasent surprise,
when he founds it's used for dump too.

You should to do something with shmalloc() and the file,
where it's described, to avoid such easter eggs.

> +	if (!page_ids) {
> +		pr_err("Failed to shmalloc page_ids\n");
> +		return -1;
> +	}
> +	atomic_set(page_ids, 1);
> +
> +	return 0;
> +}
>  
>  void up_page_ids_base(void)
>  {
> @@ -517,8 +534,8 @@ void up_page_ids_base(void)
>  	 * higher IDs.
>  	 */
>  
> -	BUG_ON(page_ids != 1);
> -	page_ids += 0x10000;
> +	BUG_ON(atomic_read(page_ids) != 1);
> +	atomic_add(0x10000, page_ids);
>  }
>  
>  struct cr_img *open_pages_image_at(int dfd, unsigned long flags, struct cr_img *pmi, u32 *id)
> @@ -531,7 +548,7 @@ struct cr_img *open_pages_image_at(int dfd, unsigned long flags, struct cr_img *
>  		pagemap_head__free_unpacked(h, NULL);
>  	} else {
>  		PagemapHead h = PAGEMAP_HEAD__INIT;
> -		*id = h.pages_id = page_ids++;
> +		*id = h.pages_id = atomic_inc_return(page_ids);
>  		if (pb_write_one(pmi, &h, PB_PAGEMAP_HEAD) < 0)
>  			return NULL;
>  	}
> diff --git a/criu/include/image.h b/criu/include/image.h
> index d9c4bdbc8da4..60e44297a858 100644
> --- a/criu/include/image.h
> +++ b/criu/include/image.h
> @@ -170,4 +170,6 @@ extern int read_img_str(struct cr_img *, char **pstr, int size);
>  
>  extern void close_image(struct cr_img *);
>  
> +extern int images_init(void);
> +
>  #endif /* __CR_IMAGE_H__ */
> -- 
> 2.13.1
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu