[PATCH] namespace: update event counter when umounting a deleted dentry

[PATCH] namespace: update event counter when umounting a deleted dentry

[Andrey Ulanov]

This change fixes a problem originally reported at:
https://github.com/coreos/bugs/issues/1153
You can find code that reproduces it at:
https://github.com/coreos/bugs/issues/1153#issuecomment-210201681

- m_start() in fs/namespace.c expects that ns->event is incremented each
  time a mount added or removed from ns->list.
- umount_tree() removes items from the list but does not increment event
  counter, expecting that it's done before the function is called.
- There are some codepaths that call umount_tree() without updating
  "event" counter. e.g. from __detach_mounts().
- When this happens m_start may reuse a cached mount structure that no
  longer belongs to ns->list (i.e. use after free which usually leads
  to infinite loop).

This change fixes the above problem by incrementing global event counter
before invoking umount_tree().

Signed-off-by: Andrey Ulanov <andreyu@google.com>
---
 fs/namespace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..b06bb9a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1562,6 +1562,7 @@ void __detach_mounts(struct dentry *dentry)
 		goto out_unlock;
 
 	lock_mount_hash();
+	event++;
 	while (!hlist_empty(&mp->m_list)) {
 		mnt = hlist_entry(mp->m_list.first, struct mount, mnt_mp_list);
 		if (mnt->mnt.mnt_flags & MNT_UMOUNT) {
@@ -1576,7 +1577,7 @@ out_unlock:
 	namespace_unlock();
 }
 
-/* 
+/*
  * Is the caller allowed to modify his namespace?
  */
 static inline bool may_mount(void)
@@ -1765,6 +1766,7 @@ void drop_collected_mounts(struct vfsmount *mnt)
 {
 	namespace_lock();
 	lock_mount_hash();
+	event++;
 	umount_tree(real_mount(mnt), UMOUNT_SYNC);
 	unlock_mount_hash();
 	namespace_unlock();
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] namespace: update event counter when umounting a deleted dentry

[Al Viro]

On Thu, Apr 14, 2016 at 04:48:32PM -0700, Andrey Ulanov wrote:
> +++ b/fs/namespace.c
> @@ -1562,6 +1562,7 @@ void __detach_mounts(struct dentry *dentry)
>  		goto out_unlock;
>  
>  	lock_mount_hash();
> +	event++;

That part makes sense.

> -/* 
> +/*
>   * Is the caller allowed to modify his namespace?
>   */
>  static inline bool may_mount(void)
> @@ -1765,6 +1766,7 @@ void drop_collected_mounts(struct vfsmount *mnt)
>  {
>  	namespace_lock();
>  	lock_mount_hash();
> +	event++;

... but what the hell is this one for?

[PATCH] namespace: update event counter when umounting a deleted dentry

[Andrey Ulanov]

- m_start() in fs/namespace.c expects that ns->event is incremented each
  time a mount added or removed from ns->list.
- umount_tree() removes items from the list but does not increment event
  counter, expecting that it's done before the function is called.
- There are some codepaths that call umount_tree() without updating
  "event" counter. e.g. from __detach_mounts().
- When this happens m_start may reuse a cached mount structure that no
  longer belongs to ns->list (i.e. use after free which usually leads
  to infinite loop).

This change fixes the above problem by incrementing global event counter
before invoking umount_tree().

Change-Id: I622c8e84dcb9fb63542372c5dbf0178ee86bb589
Signed-off-by: Andrey Ulanov <andreyu@google.com>
---
 fs/namespace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..72508d2 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1562,6 +1562,7 @@ void __detach_mounts(struct dentry *dentry)
 		goto out_unlock;
 
 	lock_mount_hash();
+	event++;
 	while (!hlist_empty(&mp->m_list)) {
 		mnt = hlist_entry(mp->m_list.first, struct mount, mnt_mp_list);
 		if (mnt->mnt.mnt_flags & MNT_UMOUNT) {
@@ -1576,7 +1577,7 @@ out_unlock:
 	namespace_unlock();
 }
 
-/* 
+/*
  * Is the caller allowed to modify his namespace?
  */
 static inline bool may_mount(void)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] namespace: update event counter when umounting a deleted dentry

[Andrey Ulanov]

On Thu, Apr 14, 2016 at 5:50 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> +     event++;
> ... but what the hell is this one for?

You're right this wasn't necessary. See updated patch.