[CRIU] [PATCH] mnt: Carry run-time device ID in mount_info

[Pavel Emelyanov]

On 12/03/2015 03:39 PM, Cyrill Gorcunov wrote:
> On Thu, Dec 03, 2015 at 03:01:11PM +0300, Pavel Emelyanov wrote:
>>> +	/*
>>> +	 * To prevent overmount clash we use two entries
>>> +	 * @s_dev and @s_dev_rt.  On dump @s_dev = @s_dev_rt
>>> +	 * obtained from procfs parsing but on restore @s_dev_rt
>>> +	 * may differ with one obtained from the image: the device
>>> +	 * ID may be new on provided by the kernel itself on new
>>> +	 * mount.
>>> +	 */
>>> +	if (dev != pm->s_dev && dev != pm->s_dev_rt) {
>>
>> The first comparison is wrong, a "new" device can by chance coincide with
>> some "old" one and we'll have erroneous "open_mountpoint is OK".
> 
> How is that? There is two points where @s_dev and @s_dev_rt are
> assigned
> 
> 1) When we parse procfs => @s_dev = @s_dev_rt always
> 2) When we do mount the device and fetch new run-time @s_dev_rt
>    from the stat call, so I really don't see how coincide may happen/

Consider you restore two mountpoints that have devices 1.11 and 2.22 in 
the images. When you mount them they get 2.22 and 3.33 devices respectively.

Now you try to open the 2nd mountpoint, but due to the BUG() in criu code
you effectively open the 1st one. So with your patch you will compare

	if (opened_dev != 2.22 and opened_dev != 3.33)
		goto error.

and will NOT detect this error, but should.

>> Also here should go BUG_ON(pm->s_dev_rt == NON_INITIALIZED).
> 
> sure
> 
>>
>>> +		pr_err("The file system %#x %#x (%#x) %s %s is inaccessible\n",
>>> +		       pm->s_dev, pm->s_dev_rt, (int)dev,
>>> +		       pm->fstype->name, pm->ns_mountpoint);
>>>  		goto err;
>>>  	}
>>>  
>>> @@ -1795,9 +1804,21 @@ static char *resolve_source(struct mount_info *mi)
>>>  
>>>  static int restore_shared_options(struct mount_info *mi, bool private, bool shared, bool slave)
>>>  {
>>> +	struct stat st;
>>> +
>>>  	pr_debug("%d:%s private %d shared %d slave %d\n",
>>>  			mi->mnt_id, mi->mountpoint, private, shared, slave);
>>>  
>>> +	/*
>>> +	 * Fetch runtime device once we're mounted for mount
>>> +	 * resolving when device has been migrated.
>>> +	 */
>>> +	if (stat(mi->mountpoint, &st)) {
>>> +		pr_perror("Can't stat on %s\n", mi->mountpoint);
>>> +		return -1;
>>> +	}
>>> +	mi->s_dev_rt = MKKDEV(major(st.st_dev), minor(st.st_dev));
>>
>> This stat() should in in do_mount_one() after the 
> 
> Why? Every mount point created go through restore_shared_options, don't they?

Yes, but it's quite strange that a routine that has to restore shared options
(that's its name) suddenly picks up the device maj:min pair.

-- Pavel