Re: [PATCH 01/34] VFS: Make clone_mnt() and copy_tree() return error codes

[Valerie Aurora <vaurora@redhat.com>]

On Fri, Oct 01, 2010 at 11:32:43AM -0700, Ram Pai wrote:
> On Fri, Oct 01, 2010 at 11:12:48AM +0200, Szeredi Miklos wrote:
> > On Fri, Oct 1, 2010 at 3:58 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > > > > > > @@ -1212,11 +1216,12 @@ struct vfsmount *copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> > > > > > > ? ? ? ? struct path path;
> > > > > > >
> > > > > > > ? ? ? ? if (!(flag & CL_COPY_ALL) && IS_MNT_UNBINDABLE(mnt))
> > > > > > > - ? ? ? ? ? ? ? return NULL;
> > > > > > > + ? ? ? ? ? ? ? return ERR_PTR(-EINVAL);
> > > > >
> > > > > Ram, do you remember how this worked?
> > > >
> > > > Oops. That should be a OR condition. there is one other check in that
> > > > function that should also be a OR condition.
> > >
> > > I may be wrong here. Can't exactly recollect what CL_COPY_ALL flag means. Al Viro
> > > might remember? ?If CL_COPY_ALL means, to clone everything irrespective of any other
> > > flags, then the above code seems right.
> > 
> > CL_COPY_ALL means clone the mount despite MNT_UNBINDABLE.  It is used
> > for cloning the whole namespace and for collect_mounts(), both of
> > which ignore MNT_UNBINDABLE.
> 
> Ok. That reminds me  when the above piece of code in copy_tree() is triggered.
> It triggered when a mount tree with a unbindable mount at its head
> is moved on a shared mount with atleast one peer.
> 
> something like this should trigger the code.
> 
> # create a unbindable mount
> mkdir -p /mnt2/m1
> mount --bind /mnt2/m1 /mnt2/m1
> mount --make-unbindable /mnt2/m1
> 
> #create a shared mount with one peer.
> mkdir -p /mnt2/s1
> mkdir -p /mnt2/s2
> mount --bind /mnt2/s1 /mnt2/s1
> mount --make-shared /mnt2/s1
> mount --bind /mnt2/s1 /mnt2/s2
> 
> #move the unbindable mount to one of the shared peer
> mkdir -p /mnt2/s1/movemount
> mount --move /mnt2/m1 /mnt2/s1/movemount
> 
> the last step will fail and that is because of the above check in copy_tree()

Actually, it fails in do_move_mount(), as Miklos theorized.  I tested
it with the above in an attempt to trigger it in practice in case the
code review was wrong, but failed.

> > Of the two remaining callers of copy_tree() do_loopback already checks
> > MNT_UNBINDABLE on the root of the tree to be copied.
> > 
> > So that leaves the one in pnode.c.   That one will be called when
> > attaching a new mount or mount tree.  If the root of that tree is
> > unbindable then the propagation will fail with -ENOMEM which is wrong,
> > it should simply skip the whole tree and not try to propagate.  
> 
> Yes.  the propagation_mnt() should fail if it is unable to create clones
> of the source mount due to any reason. However -ENOMEM may not be
> the right return code. 
> 
> 
> > Calls
> > which result in propagation are  do_loopback, do_move_mount and
> > do_add_mount.  Of this do_loopback and do_move_mount already check for
> > MNT_UNBINDABLE, do_add_mount doesn't check, but should probably just
> > mask out MNT_UNBINDABLE.
> > 
> > So in the end that check in copy_tree() should never actually trigger
> > and can be turned into a WARN_ON
> 
> You can do that. But then we have to catch for the cases where a unbindable
> mount is moved on a shared mounts. I suppose we can put in a check in do_move_mount().
> > 
> > Additionally the propagation code should perhaps be more defensive and
> > skip MNT_UNBINDABLE source mounts.
> 
> No. If we do that, I am afraid, we will end up with inconsistent peer-mount trees 
> which will not resemble each other.

Any chance you have the time to do a little documentation on where
checks should be done and what flags each function expects?  Right now
the distribution and location of the checks tend to send the reader
off on false trails...

-VAL