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

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

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>
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 <viro@zeniv.linux.org.uk>

---

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

aceb5c8912a85abd7a1a0f704bc742936cdad880
diff --git a/fs/namespace.c b/fs/namespace.c
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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;

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Miklos Szeredi]

> - 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

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Ram Pai]

On Mon, 2005-11-07 at 22:59, Miklos Szeredi wrote:
> > - 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.

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

RP

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Miklos Szeredi]

> > 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

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Ram Pai]

On Tue, 2005-11-08 at 01:28, Miklos Szeredi wrote:
> > > 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.

 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.  

> 
> 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

> 
> Miklos

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Miklos Szeredi]

>  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?

> > 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

Re: [PATCH 2/18] cleanups and bug fix in do_loopback()

[Ram Pai]

On Wed, 2005-11-09 at 13:15, Miklos Szeredi wrote:
> >  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


> 
> > > 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