Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335759 > unrolled thread

[PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal

Back to article view | Back to linux.kernel

Contents

  [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal Maxim Patlasov <mpatlasov@virtuozzo.com> - 2016-02-16 20:50 +0100
    Re: [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-16 21:00 +0100
      Re: [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal Maxim Patlasov <mpatlasov@virtuozzo.com> - 2016-02-17 08:10 +0100
    Re: [Devel] [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal Andrew Vagin <avagin@virtuozzo.com> - 2016-02-17 00:00 +0100

#1335759 — [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal

propagate_one(m) calculates "type" argument for copy_tree() like this:

>    if (m->mnt_group_id == last_dest->mnt_group_id) {
>        type = CL_MAKE_SHARED;
>    } else {
>        type = CL_SLAVE;
>        if (IS_MNT_SHARED(m))
>           type |= CL_MAKE_SHARED;
>   }

The "type" argument then governs clone_mnt() behavior with respect to flags
and mnt_master of new mount. When we iterate through a slave group, it is
possible that both current "m" and "last_dest" are not shared (although,
both are slaves, i.e. have non-NULL mnt_master-s). Then the comparison
above erroneously makes new mount shared and sets its mnt_master to
last_source->mnt_master. The patch fixes the problem by handling zero
mnt_group_id-s as though they are unequal.

The similar problem exists in the implementation of "else" clause above
when we have to ascend upward in the master/slave tree by calling:

>    last_source = last_source->mnt_master;
>    last_dest = last_source->mnt_parent;

proper number of times. The last step is governed by
"n->mnt_group_id != last_dest->mnt_group_id" condition that may lie if
both are zero. The patch fixes this case in the same way as the former one.

Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/pnode.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/pnode.c b/fs/pnode.c
index 6367e1e..abf156a 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -212,7 +212,7 @@ static int propagate_one(struct mount *m)
 	/* skip if mountpoint isn't covered by it */
 	if (!is_subdir(mp->m_dentry, m->mnt.mnt_root))
 		return 0;
-	if (m->mnt_group_id == last_dest->mnt_group_id) {
+	if (m->mnt_group_id && m->mnt_group_id == last_dest->mnt_group_id) {
 		type = CL_MAKE_SHARED;
 	} else {
 		struct mount *n, *p;
@@ -223,7 +223,9 @@ static int propagate_one(struct mount *m)
 					last_source = last_source->mnt_master;
 					last_dest = last_source->mnt_parent;
 				}
-				if (n->mnt_group_id != last_dest->mnt_group_id) {
+				if (n->mnt_group_id != last_dest->mnt_group_id ||
+				    (!n->mnt_group_id &&
+				     !last_dest->mnt_group_id)) {
 					last_source = last_source->mnt_master;
 					last_dest = last_source->mnt_parent;
 				}

[toc] | [next] | [standalone]

#1335761

On Tue, Feb 16, 2016 at 11:45:33AM -0800, Maxim Patlasov wrote:
> propagate_one(m) calculates "type" argument for copy_tree() like this:
> 
> >    if (m->mnt_group_id == last_dest->mnt_group_id) {
> >        type = CL_MAKE_SHARED;
> >    } else {
> >        type = CL_SLAVE;
> >        if (IS_MNT_SHARED(m))
> >           type |= CL_MAKE_SHARED;
> >   }
> 
> The "type" argument then governs clone_mnt() behavior with respect to flags
> and mnt_master of new mount. When we iterate through a slave group, it is
> possible that both current "m" and "last_dest" are not shared (although,
> both are slaves, i.e. have non-NULL mnt_master-s). Then the comparison
> above erroneously makes new mount shared and sets its mnt_master to
> last_source->mnt_master. The patch fixes the problem by handling zero
> mnt_group_id-s as though they are unequal.
> 
> The similar problem exists in the implementation of "else" clause above
> when we have to ascend upward in the master/slave tree by calling:
> 
> >    last_source = last_source->mnt_master;
> >    last_dest = last_source->mnt_parent;
> 
> proper number of times. The last step is governed by
> "n->mnt_group_id != last_dest->mnt_group_id" condition that may lie if
> both are zero. The patch fixes this case in the same way as the former one.

Mind putting together a reproducer?

[toc] | [prev] | [next] | [standalone]

#1336100

[Multipart message — attachments visible in raw view] — view raw

On 02/16/2016 11:54 AM, Al Viro wrote:
> On Tue, Feb 16, 2016 at 11:45:33AM -0800, Maxim Patlasov wrote:
>> propagate_one(m) calculates "type" argument for copy_tree() like this:
>>
>>>     if (m->mnt_group_id == last_dest->mnt_group_id) {
>>>         type = CL_MAKE_SHARED;
>>>     } else {
>>>         type = CL_SLAVE;
>>>         if (IS_MNT_SHARED(m))
>>>            type |= CL_MAKE_SHARED;
>>>    }
>> The "type" argument then governs clone_mnt() behavior with respect to flags
>> and mnt_master of new mount. When we iterate through a slave group, it is
>> possible that both current "m" and "last_dest" are not shared (although,
>> both are slaves, i.e. have non-NULL mnt_master-s). Then the comparison
>> above erroneously makes new mount shared and sets its mnt_master to
>> last_source->mnt_master. The patch fixes the problem by handling zero
>> mnt_group_id-s as though they are unequal.
>>
>> The similar problem exists in the implementation of "else" clause above
>> when we have to ascend upward in the master/slave tree by calling:
>>
>>>     last_source = last_source->mnt_master;
>>>     last_dest = last_source->mnt_parent;
>> proper number of times. The last step is governed by
>> "n->mnt_group_id != last_dest->mnt_group_id" condition that may lie if
>> both are zero. The patch fixes this case in the same way as the former one.
> Mind putting together a reproducer?

There are two files attached: reproducer1.c and reproducer2.c. The 
former demonstrates the problem before applying the patch. The latter 
demonstrates why the first hunk of the patch is not enough.

[root@f22ml ~]# reproducer1
main pid = 1496
monitor pid = 1497
child pid = 1498
grand-child pid = 1499

[root@f22ml ~]# grep "child" /proc/1496/mountinfo
243 144 0:37 /child /tmp/child rw shared:93 - tmpfs tmpfs rw,seclabel
[root@f22ml ~]# grep "child" /proc/1498/mountinfo
244 208 0:37 /child /tmp/child rw shared:127 master:93 - tmpfs tmpfs 
rw,seclabel
[root@f22ml ~]# grep "child" /proc/1499/mountinfo
245 240 0:37 /child /tmp/child rw master:127 - tmpfs tmpfs rw,seclabel
[root@f22ml ~]# grep "child" /proc/1497/mountinfo
246 176 0:37 /child /tmp/child rw shared:128 master:127 - tmpfs tmpfs 
rw,seclabel

while expected info for 1497 would be:
246 176 0:37 /child /tmp/child rw master:93 - tmpfs tmpfs rw,seclabel

Now, assuming that only the first hunk of the patch is applied:

 > -    if (m->mnt_group_id == last_dest->mnt_group_id) {
 > +    if (m->mnt_group_id && m->mnt_group_id == last_dest->mnt_group_id) {

[root@f22ml ~]# reproducer2
main pid = 1506
monitor pid = 1507
child pid = 1508
grand-child pid = 1509

[root@f22ml ~]# grep "child" /proc/1506/mountinfo
243 144 0:37 /child /tmp/child rw shared:93 - tmpfs tmpfs rw,seclabel
[root@f22ml ~]# grep "child" /proc/1508/mountinfo
244 208 0:37 /child /tmp/child rw shared:93 - tmpfs tmpfs rw,seclabel
[root@f22ml ~]# grep "child" /proc/1509/mountinfo
245 240 0:37 /child /tmp/child rw master:93 - tmpfs tmpfs rw,seclabel
[root@f22ml ~]# grep "child" /proc/1507/mountinfo
246 176 0:37 /child /tmp/child rw master:0 - tmpfs tmpfs rw,seclabel

while expected info for 1507 would be:
246 176 0:37 /child /tmp/child rw master:93 - tmpfs tmpfs rw,seclabel

Thanks,
Maxim

[toc] | [prev] | [next] | [standalone]

#1335905 — Re: [Devel] [PATCH] fs/pnode.c: treat zero mnt_group_id-s as unequal

On Tue, Feb 16, 2016 at 11:45:33AM -0800, Maxim Patlasov wrote:
> propagate_one(m) calculates "type" argument for copy_tree() like this:
> 
> >    if (m->mnt_group_id == last_dest->mnt_group_id) {
> >        type = CL_MAKE_SHARED;
> >    } else {
> >        type = CL_SLAVE;
> >        if (IS_MNT_SHARED(m))
> >           type |= CL_MAKE_SHARED;
> >   }
> 
> The "type" argument then governs clone_mnt() behavior with respect to flags
> and mnt_master of new mount. When we iterate through a slave group, it is
> possible that both current "m" and "last_dest" are not shared (although,
> both are slaves, i.e. have non-NULL mnt_master-s). Then the comparison
> above erroneously makes new mount shared and sets its mnt_master to
> last_source->mnt_master. The patch fixes the problem by handling zero
> mnt_group_id-s as though they are unequal.
> 
> The similar problem exists in the implementation of "else" clause above
> when we have to ascend upward in the master/slave tree by calling:
> 
> >    last_source = last_source->mnt_master;
> >    last_dest = last_source->mnt_parent;
> 
> proper number of times. The last step is governed by
> "n->mnt_group_id != last_dest->mnt_group_id" condition that may lie if
> both are zero. The patch fixes this case in the same way as the former one.
>

Acked-by: Andrei Vagin <avagin@virtuozzo.com>

> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/pnode.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 6367e1e..abf156a 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -212,7 +212,7 @@ static int propagate_one(struct mount *m)
>  	/* skip if mountpoint isn't covered by it */
>  	if (!is_subdir(mp->m_dentry, m->mnt.mnt_root))
>  		return 0;
> -	if (m->mnt_group_id == last_dest->mnt_group_id) {
> +	if (m->mnt_group_id && m->mnt_group_id == last_dest->mnt_group_id) {
>  		type = CL_MAKE_SHARED;
>  	} else {
>  		struct mount *n, *p;
> @@ -223,7 +223,9 @@ static int propagate_one(struct mount *m)
>  					last_source = last_source->mnt_master;
>  					last_dest = last_source->mnt_parent;
>  				}
> -				if (n->mnt_group_id != last_dest->mnt_group_id) {
> +				if (n->mnt_group_id != last_dest->mnt_group_id ||
> +				    (!n->mnt_group_id &&
> +				     !last_dest->mnt_group_id)) {
>  					last_source = last_source->mnt_master;
>  					last_dest = last_source->mnt_parent;
>  				}
> 
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web