Skip to contentSkip to conversation

cleanups and bug fix in do_loopback()

PATCH In review Al Viro 3 participants 7 messages Nov 8, 2005 – Nov 10, 2005 VFS

lore
Share & tools
lore

Al Viro

author
#0 lore raw

PATCH 2/18 cleanups and bug fix in do_loopback()

From: Al Viro <[email protected]>
Date: 1131401704 -0500

- check_mnt() on the source of binding should've been unconditional from the very beginning. My fault - as far I could've trace it, that's an old thinko made back in 2001. Kudos to Miklos for spotting it... Fixed.
- code cleaned up.

Signed-off-by Al Viro <[email protected]>

---

 fs/namespace.c |   41 ++++++++++++++++++++++-------------------
 1 files changed, 22 insertions(+), 19 deletions(-)

aceb5c8912a85abd7a1a0f704bc742936cdad880

fs/namespace.c +22 -19
@@ -661,29 +661,32 @@ static int do_loopback(struct nameidata 
 
 	down_write(&current->namespace->sem);
 	err = -EINVAL;
-	if (check_mnt(nd->mnt) && (!recurse || check_mnt(old_nd.mnt))) {
-		err = -ENOMEM;
-		if (recurse)
-			mnt = copy_tree(old_nd.mnt, old_nd.dentry);
-		else
-			mnt = clone_mnt(old_nd.mnt, old_nd.dentry);
-	}
+	if (!check_mnt(nd->mnt) || !check_mnt(old_nd.mnt))
+		goto out;
+
+	err = -ENOMEM;
+	if (recurse)
+		mnt = copy_tree(old_nd.mnt, old_nd.dentry);
+	else
+		mnt = clone_mnt(old_nd.mnt, old_nd.dentry);
+
+	if (!mnt)
+		goto out;
+
+	/* stop bind mounts from expiring */
+	spin_lock(&vfsmount_lock);
+	list_del_init(&mnt->mnt_expire);
+	spin_unlock(&vfsmount_lock);
 
-	if (mnt) {
-		/* stop bind mounts from expiring */
+	err = graft_tree(mnt, nd);
+	if (err) {
 		spin_lock(&vfsmount_lock);
-		list_del_init(&mnt->mnt_expire);
+		umount_tree(mnt);
 		spin_unlock(&vfsmount_lock);
+	} else
+		mntput(mnt);
 
-		err = graft_tree(mnt, nd);
-		if (err) {
-			spin_lock(&vfsmount_lock);
-			umount_tree(mnt);
-			spin_unlock(&vfsmount_lock);
-		} else
-			mntput(mnt);
-	}
-
+out:
 	up_write(&current->namespace->sem);
 	path_release(&old_nd);
 	return err;
#1 lore raw
Quoted 5 lines

- check_mnt() on the source of binding should've been unconditional from the very beginning. My fault - as far I could've trace it, that's an old thinko made back in 2001. Kudos to Miklos for spotting it... Fixed.
- code cleaned up.

Can you please explain what purpose does this serve?

AFAICS check_mnt() was there to ensure that operations are done under the proper namespace semaphore.

Next in the series the namespace semaphore is made global, which basically means, that most of the check_mnt() invocations become useless.

The ones which as a side effect prevent grafting to a detached mount can be changed to check for (mnt->mnt_namespace == NULL) instead of check against current->namespace.

I see no other reason for wanting to prevent binds from detached mounts or other namespaces. It has been discussed that it would be a good _controlled_ way to send/receive mounts from other namespace without adding any complexity. In fact it would only be removal of complexity now.

Miklos

#2 lore raw
Quoted 18 lines · Miklos Szeredi
Quoted 5 lines

- check_mnt() on the source of binding should've been unconditional from the very beginning. My fault - as far I could've trace it, that's an old thinko made back in 2001. Kudos to Miklos for spotting it... Fixed.
- code cleaned up.

Can you please explain what purpose does this serve?

AFAICS check_mnt() was there to ensure that operations are done under the proper namespace semaphore.

Next in the series the namespace semaphore is made global, which basically means, that most of the check_mnt() invocations become useless.
The ones which as a side effect prevent grafting to a detached mount can be changed to check for (mnt->mnt_namespace == NULL) instead of check against current->namespace.

Quoted 4 lines

I see no other reason for wanting to prevent binds from detached mounts or other namespaces. It has been discussed that it would be a good _controlled_ way to send/receive mounts from other namespace without adding any complexity.

AFAICT, the ability to bind across namespaces defeats the private-ness property of per-process-namespaces.

RP

#3 lore raw
Quoted 7 lines
Quoted 4 lines

I see no other reason for wanting to prevent binds from detached mounts or other namespaces. It has been discussed that it would be a good _controlled_ way to send/receive mounts from other namespace without adding any complexity.

AFAICT, the ability to bind across namespaces defeats the private-ness property of per-process-namespaces.

No. The privateness is guaranteed by proc_check_root(), which is similar, but not the same as check_mnt(), and wich restrict _access_ to other namespaces.

check_mnt() restricts operations on other namespaces if you _already_ have access to said namespace. For example via a file descriptor sent between two processes in different namespaces.

Also with ptrace() you can still access other process's namespace, so proc_check_root() is also too strict (or ptrace() too lax).

Miklos

#4 lore raw
Quoted 8 lines · Miklos Szeredi
Quoted 7 lines
Quoted 4 lines

I see no other reason for wanting to prevent binds from detached mounts or other namespaces. It has been discussed that it would be a good _controlled_ way to send/receive mounts from other namespace without adding any complexity.

AFAICT, the ability to bind across namespaces defeats the private-ness property of per-process-namespaces.

Quoted 6 lines

No. The privateness is guaranteed by proc_check_root(), which is similar, but not the same as check_mnt(), and wich restrict _access_ to other namespaces.
check_mnt() restricts operations on other namespaces if you _already_ have access to said namespace. For example via a file descriptor sent between two processes in different namespaces.

Yes there is some contradiction of some sorts on this. private-ness
means that the namespace must _not_ be accesible to processes in other namespace. But 'file descriptor sent between two processes in different namespaces' seems to break that guarantee.

Quoted 3 lines

Also with ptrace() you can still access other process's namespace, so proc_check_root() is also too strict (or ptrace() too lax).

same here.

RP

Quoted 2 lines

Miklos

#5 lore raw
Quoted 4 lines

Yes there is some contradiction of some sorts on this. private-ness
means that the namespace must _not_ be accesible to processes in other namespace. But 'file descriptor sent between two processes in different namespaces' seems to break that guarantee.

So..., are we going to check namespace in every file operation? How much do you want to bet, that it won't break any applications?

Quoted 4 lines
Quoted 2 lines

Also with ptrace() you can still access other process's namespace, so proc_check_root() is also too strict (or ptrace() too lax).

same here.

You mean, that ptrace() _is_ too lax? Adding a namespace check to ptrace might well cause grief too.

The real question is, how private do we want the namespace to be. I don't believe, we need to make it any more private than it currently is.

Miklos

#6 lore raw
Quoted 7 lines · Miklos Szeredi
Quoted 4 lines

Yes there is some contradiction of some sorts on this. private-ness
means that the namespace must _not_ be accesible to processes in other namespace. But 'file descriptor sent between two processes in different namespaces' seems to break that guarantee.

So..., are we going to check namespace in every file operation? How much do you want to bet, that it won't break any applications?

I don't know. May be there are applications out there that depend on this. It depends on the definition of private-ness of namespace. I am just saying that you raise a valid point.

I am not sure if fixing this behavior hurts more or soothes more,

Any idea?
RP

Quoted 14 lines
Quoted 5 lines
Quoted 3 lines

Also with ptrace() you can still access other process's namespace, so proc_check_root() is also too strict (or ptrace() too lax).

same here.

You mean, that ptrace() _is_ too lax? Adding a namespace check to ptrace might well cause grief too.

The real question is, how private do we want the namespace to be. I don't believe, we need to make it any more private than it currently is.

Miklos