mount: don't overmount a mount if it should be bind-mounted somewhere

Submitted by Andrei Vagin on April 28, 2016, 8:08 p.m.

Details

Message ID 1461874101-3050-1-git-send-email-avagin@openvz.org
State Rejected
Series "mount: don't overmount a mount if it should be bind-mounted somewhere"
Headers show

Commit Message

Andrei Vagin April 28, 2016, 8:08 p.m.
From: Andrew Vagin <avagin@virtuozzo.com>

It's impossiable to make a bind-mount if a source is overmounted.

Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
---
 criu/mount.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index 0b1c2ba..9dcd7d2 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -2554,6 +2554,14 @@  static bool can_mount_now(struct mount_info *mi)
 	if (!mi->parent)
 		return true;
 
+	if (strcmp(mi->parent->mountpoint, mi->mountpoint) == 0) {
+		struct mount_info *b;
+
+		list_for_each_entry(b, &mi->parent->mnt_bind, mnt_bind)
+			if (!b->mounted)
+				return false;
+	}
+
 	if (mi->external)
 		goto shared;
 

Comments

Pavel Emelianov April 29, 2016, 11:55 a.m.
On 04/28/2016 11:08 PM, Andrey Vagin wrote:
> From: Andrew Vagin <avagin@virtuozzo.com>
> 
> It's impossiable to make a bind-mount if a source is overmounted.
> 
> Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
> ---
>  criu/mount.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index 0b1c2ba..9dcd7d2 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -2554,6 +2554,14 @@ static bool can_mount_now(struct mount_info *mi)
>  	if (!mi->parent)
>  		return true;
>  
> +	if (strcmp(mi->parent->mountpoint, mi->mountpoint) == 0) {
> +		struct mount_info *b;
> +
> +		list_for_each_entry(b, &mi->parent->mnt_bind, mnt_bind)
> +			if (!b->mounted)
> +				return false;

Source is overmounted and we say "can't mount now", that's OK. But where's
the guarantee that the source will stop being such, so we'll be able to truen
true from here?

> +	}
> +
>  	if (mi->external)
>  		goto shared;
>  
>
Andrey Vagin April 29, 2016, 3:21 p.m.
On Fri, Apr 29, 2016 at 02:55:57PM +0300, Pavel Emelyanov wrote:
> On 04/28/2016 11:08 PM, Andrey Vagin wrote:
> > From: Andrew Vagin <avagin@virtuozzo.com>
> > 
> > It's impossiable to make a bind-mount if a source is overmounted.
> > 
> > Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> > Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
> > ---
> >  criu/mount.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/criu/mount.c b/criu/mount.c
> > index 0b1c2ba..9dcd7d2 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -2554,6 +2554,14 @@ static bool can_mount_now(struct mount_info *mi)
> >  	if (!mi->parent)
> >  		return true;
> >  
> > +	if (strcmp(mi->parent->mountpoint, mi->mountpoint) == 0) {
> > +		struct mount_info *b;
> > +
> > +		list_for_each_entry(b, &mi->parent->mnt_bind, mnt_bind)
> > +			if (!b->mounted)
> > +				return false;
> 
> Source is overmounted and we say "can't mount now", that's OK. But where's
> the guarantee that the source will stop being such, so we'll be able to truen
> true from here?

It's a good question, but it should not block this patch. Without this
patch we don't returen false from here and then create a wrong bind
mount. With this patch we don't do this mistake, but sometimes can
return a error that we are not able to restore a mount tree.

I'm thinking what we should add into validate_mounts() to be sure that
we will be able to restore a mount tree.
> 
> > +	}
> > +
> >  	if (mi->external)
> >  		goto shared;
> >  
> > 
>
Pavel Emelianov April 29, 2016, 3:34 p.m.
On 04/29/2016 06:21 PM, Andrew Vagin wrote:
> On Fri, Apr 29, 2016 at 02:55:57PM +0300, Pavel Emelyanov wrote:
>> On 04/28/2016 11:08 PM, Andrey Vagin wrote:
>>> From: Andrew Vagin <avagin@virtuozzo.com>
>>>
>>> It's impossiable to make a bind-mount if a source is overmounted.
>>>
>>> Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>> Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
>>> ---
>>>  criu/mount.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/criu/mount.c b/criu/mount.c
>>> index 0b1c2ba..9dcd7d2 100644
>>> --- a/criu/mount.c
>>> +++ b/criu/mount.c
>>> @@ -2554,6 +2554,14 @@ static bool can_mount_now(struct mount_info *mi)
>>>  	if (!mi->parent)
>>>  		return true;
>>>  
>>> +	if (strcmp(mi->parent->mountpoint, mi->mountpoint) == 0) {
>>> +		struct mount_info *b;
>>> +
>>> +		list_for_each_entry(b, &mi->parent->mnt_bind, mnt_bind)
>>> +			if (!b->mounted)
>>> +				return false;
>>
>> Source is overmounted and we say "can't mount now", that's OK. But where's
>> the guarantee that the source will stop being such, so we'll be able to truen
>> true from here?
> 
> It's a good question, but it should not block this patch. Without this
> patch we don't returen false from here and then create a wrong bind
> mount. With this patch we don't do this mistake, but sometimes can
> return a error that we are not able to restore a mount tree.

Failing restore after successful dump is not nice :\

> I'm thinking what we should add into validate_mounts() to be sure that
> we will be able to restore a mount tree.

Yes, please. Let's don't dump this thing rather than don't restore.

>>
>>> +	}
>>> +
>>>  	if (mi->external)
>>>  		goto shared;
>>>  
>>>
>>
> .
>