[CRIU] [PATCH] remap: fix dead pid remap of /proc/<pid>

[Pavel Emelyanov]

On 01/13/2016 04:12 PM, Tycho Andersen wrote:
> On Wed, Jan 13, 2016 at 04:05:28PM +0300, Pavel Emelyanov wrote:
>> On 01/06/2016 06:57 PM, Tycho Andersen wrote:
>>> It turns out we can't just test for /proc/<pid>, because the kernel appends
>>> (deleted), since the directory is actually deleted (vs. something like
>>> /proc/1/mountinfo, where the file still exists in the unlinked directory,
>>> so there is no (deleted)). See comment for details.
>>>
>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>> ---
>>>  files-reg.c                            | 34 +++++++++++++++++++++++++---------
>>>  test/zdtm/live/static/remap_dead_pid.c | 22 ++++++++++++++++++++--
>>>  2 files changed, 45 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/files-reg.c b/files-reg.c
>>> index 0f89be5..43872f0 100644
>>> --- a/files-reg.c
>>> +++ b/files-reg.c
>>> @@ -887,16 +887,32 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
>>>  			return -1;
>>>  		pid = strtol(start + 1, &end, 10);
>>>  
>>> -		/* if we didn't find another /, this path something
>>> -		 * like ./proc/kmsg, which we shouldn't mess with. */
>>> -		if (*end == '/') {
>>> -			*end = 0;
>>> -			ret = access(rpath, F_OK);
>>> -			*end = '/';
>>> -
>>> -			if (ret) {
>>> +		/* If strtol didn't convert anything, then we are looking at
>>> +		 * something like /proc/kmsg, which we shouldn't mess with.
>>> +		 * Anything under /proc/<pid> (including that directory itself)
>>> +		 * can be c/r'd with a dead pid remap, so let's allow all such
>>> +		 * cases.
>>> +		 */
>>> +		if (pid != 0) {
>>> +			bool is_dead = strip_deleted(link);
>>> +
>>> +			/* /proc/<pid> will be "/proc/1 (deleted)" when it is
>>> +			 * dead, but a path like /proc/1/mountinfo won't have
>>> +			 * the suffix, since it isn't actually deleted (still
>>> +			 * exists, but the parent dir is deleted). So, if we
>>> +			 * have a path like /proc/1/mountinfo, test if /proc/1
>>> +			 * exists instead, since this is what CRIU will need to
>>> +			 * open on restore.
>>> +			 */
>>> +			if (!is_dead) {
>>
>> If the is_dead is 0 here we never go dumping the pid remap, which seems to
>> be breaking the behavior on /proc/pid/xxx files.
>>
>>> +				*end = 0;
>>> +				ret = access(rpath, F_OK);
>>
>> Shouldn't this be is_dead = access() instead?
> 
> Oh, yes. Doesn't that fix the case you noted above, too?

Hm... At the first glance it should, but ...

> Also, I'll fix the test so that it doesn't use the same pid for both
> cases, so we'll see this if it breaks in the future :)

... yes, having two separate test cases would be nice :)

-- Pavel