[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