Re: [RFC][PATCH 03/20] Add vfsmount writer count

[Dave Hansen <haveblue@us.ibm.com>]

On Tue, 2006-06-20 at 22:20 +0100, Al Viro wrote:
> On Mon, Jun 19, 2006 at 10:02:16AM -0700, Dave Hansen wrote:
> > Very true.  How about this to fix it?
> > 
> > --- lxc/fs//open.c~C8.1-fix-faccesat    2006-06-19 09:59:41.000000000 -0700
> > +++ lxc-dave/fs//open.c 2006-06-19 10:01:25.000000000 -0700
> > @@ -546,8 +546,12 @@ asmlinkage long sys_faccessat(int dfd, c
> >            special_file(nd.dentry->d_inode->i_mode))
> >                 goto out_path_release;
> > 
> > -       if(__mnt_is_readonly(nd.mnt) || IS_RDONLY(nd.dentry->d_inode))
> > -               res = -EROFS;
> > +       res = mnt_want_write(nd.mnt);
> > +       if (!res) {
> > +               mnt_drop_write(nd.mnt);
> > +               if(IS_RDONLY(nd.dentry->d_inode))
> > +                       res = -EROFS;
> > +       }
> 
> So access() can make remount r/o fail?  Uh-oh...

Good point.  Thanks for catching that.

There are probably two solutions here: rework the atomics to always give
a consistent view, even during mnt_make_readonly() calls, or keep the
mnt_make_readonly() side from executing.

We're already protecting this mnt_make_readonly() from races with itself
with a write on the mnt->mnt_sb->s_umount semaphore.  We should be able
to use a read lock on the same semaphore to exclude the
mnt_make_readonly() callers.

Thoughts?

diff -puN include/linux/mount.h~C-D8-actually-add-flags include/linux/mount.h
--- robind/include/linux/mount.h~C-D8-actually-add-flags        2006-06-20 14:11:15.000000000 -0700
+++ robind-dave/include/linux/mount.h   2006-06-20 16:01:11.000000000 -0700
@@ -91,6 +91,25 @@ static inline int __mnt_is_readonly(stru
        return (atomic_read(&mnt->mnt_writers) == 0);
 }

+/*
+ * This needs to get a consistent look at mnt_writers.
+ * Without the lock, it can race against mnt_make_readonly()
+ * and mistake a temporarily decremented mnt_writers
+ * for a real read-only mount.
+ *
+ * Note: this is never suitable if you need to perform any
+ * write *operations* on the mount, only as a snapshot.
+ */
+static inline int mnt_is_readonly(struct vfsmount *mnt)
+{
+       int ret;
+
+       down_read(&mnt->mnt_sb->s_umount);
+       ret = __mnt_is_readonly(mnt);
+       up_read(&mnt->mnt_sb->s_umount);
+       return ret;
+}
+
 static inline int mnt_want_write(struct vfsmount *mnt)
 {
        int ret = 0;
--- lxc/fs/open.c~C8.1-fix-faccesat     2006-06-22 09:43:01.000000000 -0700
+++ lxc-dave/fs/open.c  2006-06-22 09:43:01.000000000 -0700
@@ -546,8 +546,12 @@ asmlinkage long sys_faccessat(int dfd, c
           special_file(nd.dentry->d_inode->i_mode))
                goto out_path_release;

-       if(__mnt_is_readonly(nd.mnt) || IS_RDONLY(nd.dentry->d_inode))
+       if(mnt_is_readonly(nd.mnt) || IS_RDONLY(nd.dentry->d_inode))
                res = -EROFS;

 out_path_release:
        path_release(&nd);


-- Dave