STRLCPY/criu

vma: Fix badly inherited FD in filemap_open

Previous patch (5a1e1aac) tried to minimize the amount of
open()s called when mmap()ing the files. Unfortunatley, there
was a mistake and wrong flags were compared which resulted in
the whole optimization working randomly (typically not
working).

Fixing the flags comparison revealed another problem. The
patch in question correllated with the 03e8c417 one, which
caused some vmas to be opened and mmaped much later than the
premap. When hitting the situation when vmas sharing their
fds are partially premapped and partially not, the whole
vm_open sharing became broken in multiple places -- either
needed fd was not opened, or the not needed left un-closed.

To fix this the context, that tracks whether the fd should
be shared or not, should be moved from collect stage to
the real opening loop. In this case we need to explicitly
know which vmas _may_ share fds (file private and shared)
with each other, so the sharing knowledge becomes spread
between open_filemap() and its callers. Oh, well...

Signed-off-by: Pavel Emelyanov <[email protected]>
Signed-off-by: Andrei Vagin <[email protected]>

Pavel Emelyanov committed 7 years ago

f0a87835

1 parent a0738c75

Revision indexing in progress... (symbol navigation in revisions will be accurate after indexed)

Total 6 files Show one by one

■ ■ ■ ■ ■ ■

criu/files-reg.c

		skipped 1623 lines
1624	1624		return open_reg_fd(fd);
1625	1625		}
1626	1626
1627		-	static int borrow_filemap(int pid, struct vma_area *vma)
	1627	+	struct filemap_ctx {
	1628	+	u32 flags;
	1629	+	struct file_desc *desc;
	1630	+	int fd;
	1631	+	/*
	1632	+	* Whether or not to close the fd when we're about to
	1633	+	* put a new one into ctx.
	1634	+	*
	1635	+	* True is used by premap, so that it just calls vm_open
	1636	+	* in sequence, immediatelly mmap()s the file, then it
	1637	+	* can be closed.
	1638	+	*
	1639	+	* False is used by open_vmas() which pre-opens the files
	1640	+	* for restorer, and the latter mmap()s them and closes.
	1641	+	*
	1642	+	* ...
	1643	+	*/
	1644	+	bool close;
	1645	+	/* ...
	1646	+	*
	1647	+	* but closing all vmas won't work, as some of them share
	1648	+	* the descriptor, so only the ones that terminate the
	1649	+	* fd-sharing chain are marked with VMA_CLOSE flag, saying
	1650	+	* restorer to close the vma's fd.
	1651	+	*
	1652	+	* Said that, this vma pointer references the previously
	1653	+	* seen vma, so that once fd changes, this one gets the
	1654	+	* closing flag.
	1655	+	*/
	1656	+	struct vma_area *vma;
	1657	+	};
	1658	+
	1659	+	static struct filemap_ctx ctx;
	1660	+
	1661	+	void filemap_ctx_init(bool auto_close)
1628	1662		{
1629		-	struct vma_area *fvma = vma->fvma;
	1663	+	ctx.desc = NULL; /* to fail the first comparison in open_ */
	1664	+	ctx.fd = -1; /* not to close random fd in _fini */
	1665	+	ctx.vma = NULL; /* not to put spurious VMA_CLOSE in _fini */
	1666	+	/* flags may remain any */
	1667	+	ctx.close = auto_close;
	1668	+	}
1630	1669
1631		-	BUG_ON(!(fvma->e->status & VMA_NO_CLOSE));
1632		-	vma->e->fd = fvma->e->fd;
1633		-
1634		-	return 0;
	1670	+	void filemap_ctx_fini(void)
	1671	+	{
	1672	+	if (ctx.close) {
	1673	+	if (ctx.fd >= 0)
	1674	+	close(ctx.fd);
	1675	+	} else {
	1676	+	if (ctx.vma)
	1677	+	ctx.vma->e->status \|= VMA_CLOSE;
	1678	+	}
1635	1679		}
1636	1680
1637	1681		static int open_filemap(int pid, struct vma_area *vma)
		skipped 10 lines
1648	1692		BUG_ON((vma->vmfd == NULL) \|\| !vma->e->has_fdflags);
1649	1693		flags = vma->e->fdflags;
1650	1694
1651		-	ret = open_path(vma->vmfd, do_open_reg_noseek_flags, &flags);
1652		-	if (ret < 0)
1653		-	return ret;
	1695	+	if (ctx.flags != flags \|\| ctx.desc != vma->vmfd) {
	1696	+	ret = open_path(vma->vmfd, do_open_reg_noseek_flags, &flags);
	1697	+	if (ret < 0)
	1698	+	return ret;
	1699	+
	1700	+	filemap_ctx_fini();
1654	1701
1655		-	vma->e->fd = ret;
	1702	+	ctx.flags = flags;
	1703	+	ctx.desc = vma->vmfd;
	1704	+	ctx.fd = ret;
	1705	+	}
	1706	+
	1707	+	ctx.vma = vma;
	1708	+	vma->e->fd = ctx.fd;
1656	1709		return 0;
1657	1710		}
1658	1711
1659		-	int collect_filemap(struct vma_area vma, struct vma_file_ctx ctx)
	1712	+	int collect_filemap(struct vma_area *vma)
1660	1713		{
1661	1714		struct file_desc *fd;
1662	1715
		skipped 12 lines
1675	1728		return -1;
1676	1729
1677	1730		vma->vmfd = fd;
1678		-	if (ctx->vma && ctx->flags == vma->e->flags && ctx->fd == fd) {
1679		-	vma->vm_open = borrow_filemap;
1680		-	vma->fvma = ctx->vma;
1681		-	ctx->vma->e->status \|= VMA_NO_CLOSE;
1682		-	/* Change VMA so that next borrower sets NO_CLOSE on us */
1683		-	ctx->vma = vma;
1684		-	} else {
1685		-	vma->vm_open = open_filemap;
1686		-	ctx->flags = vma->e->fdflags;
1687		-	ctx->fd = fd;
1688		-	ctx->vma = vma;
1689		-	}
1690		-
	1731	+	vma->vm_open = open_filemap;
1691	1732		return 0;
1692	1733		}
1693	1734
		skipped 83 lines

■ ■ ■ ■ ■ ■

criu/include/files-reg.h

		skipped 39 lines
40	40
41	41		extern struct file_desc *try_collect_special_file(u32 id, int optional);
42	42		#define collect_special_file(id) try_collect_special_file(id, 0)
43		-	struct vma_file_ctx {
44		-	u32 flags;
45		-	struct file_desc *fd;
46		-	struct vma_area *vma;
47		-	};
48		-	extern int collect_filemap(struct vma_area , struct vma_file_ctx ctx);
	43	+	extern int collect_filemap(struct vma_area *);
	44	+	extern void filemap_ctx_init(bool auto_close);
	45	+	extern void filemap_ctx_fini(void);
49	46
50	47		extern int collect_remaps_and_regfiles(void);
51	48
		skipped 11 lines

■ ■ ■ ■ ■ ■

criu/include/image.h

		skipped 88 lines
89	89		#define VMA_AREA_VVAR (1 << 12)
90	90		#define VMA_AREA_AIORING (1 << 13)
91	91
92		-	#define VMA_NO_CLOSE (1 << 28)
	92	+	#define VMA_CLOSE (1 << 28)
93	93		#define VMA_NO_PROT_WRITE (1 << 29)
94	94		#define VMA_PREMMAPED (1 << 30)
95	95		#define VMA_UNSUPP (1 << 31)
		skipped 78 lines

■ ■ ■ ■ ■ ■

criu/include/vma.h

		skipped 52 lines
53	53		int (vm_open)(int pid, struct vma_area vma);
54	54		struct file_desc *vmfd;
55	55		struct vma_area pvma; / parent for inherited VMAs */
56		-	struct vma_area fvma; / vma from which to borrow a file */
57	56		unsigned long page_bitmap; / existent pages */
58	57		unsigned long premmaped_addr; /* restore only */
59	58
		skipped 69 lines

■ ■ ■ ■ ■ ■

criu/mem.c

		skipped 449 lines
450	450		int ret = -1, vn = 0;
451	451		struct cr_img *img;
452	452		struct rst_info *ri = rsti(i);
453		-	struct vma_file_ctx ctx = {};
454	453
455	454		img = open_image(CR_FD_MM, O_RSTR, pid);
456	455		if (!img)
		skipped 53 lines
510	509		ret = collect_shmem(pid, vma);
511	510		else if (vma_area_is(vma, VMA_FILE_PRIVATE) \|\|
512	511		vma_area_is(vma, VMA_FILE_SHARED))
513		-	ret = collect_filemap(vma, &ctx);
	512	+	ret = collect_filemap(vma);
514	513		else if (vma_area_is(vma, VMA_AREA_SOCKET))
515	514		ret = collect_socket_map(vma);
516	515		else
		skipped 159 lines
676	675		pr_perror("Unable to map ANON_VMA");
677	676		return -1;
678	677		}
679		-
680		-	if (vma_area_is(vma, VMA_FILE_PRIVATE) &&
681		-	!vma_area_is(vma, VMA_NO_CLOSE))
682		-	close(vma->e->fd);
683	678		} else {
684	679		void *paddr;
685	680
		skipped 67 lines
753	748		int ret = 0;
754	749		LIST_HEAD(empty);
755	750
	751	+	filemap_ctx_init(true);
	752	+
756	753		list_for_each_entry(vma, &vmas->h, list) {
757	754		if (pstart > vma->e->start) {
758	755		ret = -1;
		skipped 28 lines
787	784		if (ret < 0)
788	785		break;
789	786		}
	787	+
	788	+	filemap_ctx_fini();
790	789
791	790		return ret;
792	791		}
		skipped 286 lines
1079	1078		struct vma_area *vma;
1080	1079		struct vm_area_list *vmas = &rsti(t)->vmas;
1081	1080
	1081	+	filemap_ctx_init(false);
	1082	+
1082	1083		list_for_each_entry(vma, &vmas->h, list) {
1083	1084		if (!vma_area_is(vma, VMA_AREA_REGULAR) \|\| !vma->vm_open)
1084	1085		continue;
		skipped 6 lines
1091	1092		pr_err("`- Can't open vma\n");
1092	1093		return -1;
1093	1094		}
	1095	+
	1096	+	/*
	1097	+	* File mappings have vm_open set to open_filemap which, in
	1098	+	* turn, puts the VMA_CLOSE bit itself. For all the rest we
	1099	+	* need to put it by hads, so that the restorer closes the fd
	1100	+	*/
	1101	+	if (!(vma_area_is(vma, VMA_FILE_PRIVATE) \|\|
	1102	+	vma_area_is(vma, VMA_FILE_SHARED)))
	1103	+	vma->e->status \|= VMA_CLOSE;
1094	1104		}
	1105	+
	1106	+	filemap_ctx_fini();
1095	1107
1096	1108		return 0;
1097	1109		}
		skipped 42 lines

■ ■ ■ ■ ■ ■

criu/pie/restorer.c

		skipped 618 lines
619	619		vma_entry->pgoff);
620	620
621	621		if ((vma_entry->fd != -1) &&
622		-	!(vma_entry->status & VMA_NO_CLOSE))
	622	+	(vma_entry->status & VMA_CLOSE))
623	623		sys_close(vma_entry->fd);
624	624
625	625		return addr;
		skipped 969 lines