[CRIU] [PATCH v3 1/6] files-reg: Account number of unlinked parent components of ghost file

Kirill Tkhai ktkhai at virtuozzo.com
Mon Jan 25 02:28:00 PST 2016



On 25.01.2016 12:54, Pavel Emelyanov wrote:
> On 01/19/2016 08:07 PM, Kirill Tkhai wrote:
>> On 18.01.2016 21:05, Pavel Emelyanov wrote:
>>> On 12/29/2015 05:05 PM, Kirill Tkhai wrote:
>>>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>>>> ---
>>>>  files-reg.c               |   69 ++++++++++++++++++++++++++++++++++++++-------
>>>>  protobuf/ghost-file.proto |    1 +
>>>>  2 files changed, 60 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/files-reg.c b/files-reg.c
>>>> index 0f89be5..3e09ae0 100644
>>>> --- a/files-reg.c
>>>> +++ b/files-reg.c
>>>> @@ -48,6 +48,7 @@ struct ghost_file {
>>>>  
>>>>  	u32			dev;
>>>>  	u32			ino;
>>>> +	u32			nr_up;
>>>>  
>>>>  	struct file_remap	remap;
>>>>  };
>>>> @@ -223,6 +224,7 @@ static int open_remap_ghost(struct reg_file_info *rfi,
>>>>  	gf->dev = gfe->dev;
>>>>  	gf->ino = gfe->ino;
>>>>  	gf->remap.rmnt_id = rfi->rfe->mnt_id;
>>>> +	gf->nr_up = gfe->has_nr_up ? gfe->nr_up : 0;
>>>>  
>>>>  	if (S_ISDIR(gfe->mode))
>>>>  		strncpy(gf->remap.rpath, rfi->path, PATH_MAX);
>>>> @@ -526,7 +528,50 @@ static struct collect_image_info remap_cinfo = {
>>>>  	.collect = collect_one_remap,
>>>>  };
>>>>  
>>>> -static int dump_ghost_file(int _fd, u32 id, const struct stat *st, dev_t phys_dev)
>>>> +static int count_unlinked_parents(GhostFileEntry *gfe, const char *path, int mntns_root)
>>>> +{
>>>> +	struct stat st;
>>>> +	char buf[PATH_MAX], *p;
>>>> +	int count = 0;
>>>> +
>>>> +	strcpy(buf, path);
>>>> +
>>>> +	p = strrchr(buf, '/');
>>>> +	if (!p) {
>>>> +		pr_err("No parent for %s", buf);
>>>> +		count = -1;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	while (p > buf) {
>>>> +		*p = '\0';
>>>> +		if (fstatat(mntns_root, buf, &st, 0) == 0)
>>>
>>> This place looks dangerous. Consider we get here after we've fstat()-ed the fd
>>> and found out that its n_link is 0. Then we go on and stat() it by path. But
>>> the path in question may refer to some other existing file and thus this whole
>>> logic becomes flawed.
>>
>> Below code counts stable parents, i.e. existing directories only. How about this?
>>
>> ---
>> diff --git a/files-reg.c b/files-reg.c
>> index ea66af0..8fdab57 100644
>> --- a/files-reg.c
>> +++ b/files-reg.c
>> @@ -48,6 +48,7 @@ struct ghost_file {
>>  
>>  	u32			dev;
>>  	u32			ino;
>> +	u32			nr_stable;
>>  
>>  	struct file_remap	remap;
>>  };
>> @@ -223,6 +224,7 @@ static int open_remap_ghost(struct reg_file_info *rfi,
>>  	gf->dev = gfe->dev;
>>  	gf->ino = gfe->ino;
>>  	gf->remap.rmnt_id = rfi->rfe->mnt_id;
>> +	gf->nr_stable = gfe->has_nr_stable ? gfe->nr_stable : 0;
>>  
>>  	if (S_ISDIR(gfe->mode))
>>  		strncpy(gf->remap.rpath, rfi->path, PATH_MAX);
>> @@ -526,7 +528,47 @@ static struct collect_image_info remap_cinfo = {
>>  	.collect = collect_one_remap,
>>  };
>>  
>> -static int dump_ghost_file(int _fd, u32 id, const struct stat *st, dev_t phys_dev)
>> +static int count_stable_parents(GhostFileEntry *gfe, char *path, int mntns_root)
>> +{
>> +	int ret = 0, count = 0;
>> +	struct stat64 st;
>> +	char *p = path;
>> +
>> +	while (1) {
>> +		p = strchr(p, '/');
>> +		if (!p)
>> +			break;
>> +		*p = '\0';
>> +
>> +		ret = fstatat64(mntns_root, path, &st, AT_SYMLINK_NOFOLLOW);
>> +		if (ret < 0 && errno != ENOENT) {
>> +			pr_err("Can't stat on %s\n", path);
>> +			*p = '/';
>> +			goto out;
> 
> Once you meet the "alive" directory, you should also compare the device
> it sits on and mountpoint it's visible via with those on the ghost file
> you see via descriptor.

Some clarification about this.

mount /dev/XXX /
mount /dev/YYY /mnt

open("/mnt/dir/file")
unlink("/mnt/dir/file")
rmdir("/mnt/dir")

mount /dev/ZZZ /mnt
ls /mnt/dir ==> -ENOENT

If we have such configuration, which directory should we consider as "stable"?
Should we unhide /dev/YYY and restore ghost file on it?

Thanks,
Kirill

>> +		}
>> +
>> +		*p = '/';
>> +
>> +		if (ret < 0)
>> +			break;
>> +		if ((st.st_mode & S_IFMT) != S_IFDIR)
>> +			break;
>> +		count++;
>> +		p++;
>> +	}
>> +
>> +	if (p && count) {
>> +		gfe->has_nr_stable = true;
>> +		gfe->nr_stable = count;
>> +		pr_debug("Ghost file %s has %d stable parent component(s)", path, count);
>> +	}
>> +	ret = 0;
>> +out:
>> +	return ret;
>> +}
>> +
>> +static int dump_ghost_file(char *path, int _fd, u32 id, const struct stat *st,
>> +			   dev_t phys_dev, int mntns_root)
>>  {
>>  	struct cr_img *img;
>>  	GhostFileEntry gfe = GHOST_FILE_ENTRY__INIT;
>> @@ -553,6 +595,9 @@ static int dump_ghost_file(int _fd, u32 id, const struct stat *st, dev_t phys_de
>>  	gfe.dev = phys_dev;
>>  	gfe.ino = st->st_ino;
>>  
>> +	if (count_stable_parents(&gfe, path, mntns_root) < 0)
>> +		return -1;
>> +
>>  	if (S_ISCHR(st->st_mode) || S_ISBLK(st->st_mode)) {
>>  		gfe.has_rdev = true;
>>  		gfe.rdev = st->st_rdev;
>> @@ -616,8 +661,8 @@ struct file_remap *lookup_ghost_remap(u32 dev, u32 ino)
>>  	return NULL;
>>  }
>>  
>> -static int dump_ghost_remap(char *path, const struct stat *st,
>> -				int lfd, u32 id, struct ns_id *nsid)
>> +static int dump_ghost_remap(char *path, const struct stat *st, int lfd,
>> +			    u32 id, struct ns_id *nsid, int mntns_root)
>>  {
>>  	struct ghost_file *gf;
>>  	RemapFilePathEntry rpe = REMAP_FILE_PATH_ENTRY__INIT;
>> @@ -645,7 +690,7 @@ static int dump_ghost_remap(char *path, const struct stat *st,
>>  	gf->id = ghost_file_ids++;
>>  	list_add_tail(&gf->list, &ghost_files);
>>  
>> -	if (dump_ghost_file(lfd, gf->id, st, phys_dev))
>> +	if (dump_ghost_file(path + 1, lfd, gf->id, st, phys_dev, mntns_root))
>>  		return -1;
>>  
>>  dump_entry:
>> @@ -934,6 +979,10 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
>>  		return 0;
>>  	}
>>  
>> +	mntns_root = mntns_get_root_fd(nsid);
>> +	if (mntns_root < 0)
>> +		return -1;
>> +
>>  	if (ost->st_nlink == 0) {
>>  		/*
>>  		 * Unpleasant, but easy case. File is completely invisible
>> @@ -942,7 +991,8 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
>>  		 * also open.
>>  		 */
>>  		strip_deleted(link);
>> -		return dump_ghost_remap(rpath + 1, ost, lfd, id, nsid);
>> +
>> +		return dump_ghost_remap(rpath + 1, ost, lfd, id, nsid, mntns_root);
>>  	}
>>  
>>  	if (nfs_silly_rename(rpath, parms)) {
>> @@ -957,10 +1007,6 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
>>  		return dump_linked_remap(rpath + 1, plen - 1, ost, lfd, id, nsid);
>>  	}
>>  
>> -	mntns_root = mntns_get_root_fd(nsid);
>> -	if (mntns_root < 0)
>> -		return -1;
>> -
>>  	ret = fstatat(mntns_root, rpath, &pst, 0);
>>  	if (ret < 0) {
>>  		/*
>> diff --git a/protobuf/ghost-file.proto b/protobuf/ghost-file.proto
>> index db056db..d74f195 100644
>> --- a/protobuf/ghost-file.proto
>> +++ b/protobuf/ghost-file.proto
>> @@ -11,4 +11,5 @@ message ghost_file_entry {
>>  	optional uint32		rdev		= 6 [(criu).dev = true, (criu).odev = true];
>>  	optional timeval	atim		= 7;
>>  	optional timeval	mtim		= 8;
>> +	optional uint32		nr_stable	= 9; /* Number of stable parent components */
>>  }
>> .
>>
> 


More information about the CRIU mailing list