fs: fix i_writecount on shmem and friends

VM_DENYWRITE currently relies on i_writecount. Unless there's an active writable reference to an inode, VM_DENYWRITE is not allowed. Unfortunately, alloc_file() does not increase i_writecount, therefore, does not prevent a following VM_DENYWRITE even though the new file might have been opened with FMODE_WRITE. However, callers of alloc_file() expect the file object to be fully instantiated so they can call fput() on it. We could now either fix all callers to do an get_write_access() if opened with FMODE_WRITE, or simply fix alloc_file() to do that. I chose the latter.

Note that this bug allows some rather subtle misbehavior. The following sequence of calls should work just fine, but currently fails:

    int p[2], orig, ro, rw;
    char buf[128];

    pipe(p);
    sprintf(buf, "/proc/self/fd/%d", p[1]);
    ro = open("/proc/self/fd/$orig", O_RDONLY);
    close(p[1]);
    rw = open("/proc/self/fd/$ro", O_RDWR);

The final open() cannot succeed as close(p[1]) caused an integer underflow on i_writecount, effectively causing VM_DENYWRITE on the inode. The open will fail with -ETXTBUSY.

It's a rather odd sequence of calls and given that open() doesn't use alloc_file() (and thus not affected by this bug), it's rather unlikely that this is a serious issue. But stuff like anon_inode shares a *single* inode across a huge set of interfaces. If any of these is broken like pipe(), it will affect all of these (ranging from dma-buf to epoll).

Cc Al Viro <[email protected]>

Cc David Howells <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 fs/file_table.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

fs/file_table.c +18 -10

index 5fff903..e3c8dd0 100644
@@ -167,6 +167,7 @@ struct file *alloc_file(struct path *path, fmode_t mode,
                const struct file_operations *fop)
 {
        struct file *file;
+       int error;
 
        file = get_empty_filp();
        if (IS_ERR(file))

@@ -178,15 +179,23 @@ struct file *alloc_file(struct path *path, fmode_t mode,
        file->f_mode = mode;
        file->f_op = fop;
 
-       /*
-        * These mounts don't really matter in practice
-        * for r/o bind mounts.  They aren't userspace-
-        * visible.  We do this for consistency, and so
-        * that we can do debugging checks at __fput()
-        */
-       if ((mode & FMODE_WRITE) && !special_file(path->dentry->d_inode->i_mode)) {
-               file_take_write(file);
-               WARN_ON(mnt_clone_write(path->mnt));
+       if (mode & FMODE_WRITE) {
+               error = get_write_access(path->dentry->d_inode);
+               if (error) {
+                       put_filp(file);
+                       return ERR_PTR(error);
+               }
+
+               /*
+                * These mounts don't really matter in practice
+                * for r/o bind mounts.  They aren't userspace-
+                * visible.  We do this for consistency, and so
+                * that we can do debugging checks at __fput()
+                */
+               if (!special_file(path->dentry->d_inode->i_mode)) {
+                       file_take_write(file);
+                       WARN_ON(mnt_clone_write(path->mnt));
+               }
        }
        if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
                i_readcount_inc(path->dentry->d_inode);
--

1.9.0

which returns ETXTBSY (most easily seen by just stracing it).

The patch would also seem to make sense, with the i_readcount_inc() being immediately below for the FMODE_READ case.

I think it's trying to fix the problem in the wrong place. The bug is real, all right, but it's not that alloc_file() for non-regulars doesn't grab writecount; it's that drop_file_write_access() drops it for those.

What the hell would we want to play with that counter for, anyway? It's not as if they could be mmapped, so all it does is making pipe(2) and socket(2) more costly, for no visible reason.

Hi

Quoted 7 lines

I think it's trying to fix the problem in the wrong place. The bug is real, all right, but it's not that alloc_file() for non-regulars doesn't grab writecount; it's that drop_file_write_access() drops it for those.

What the hell would we want to play with that counter for, anyway? It's not as if they could be mmapped, so all it does is making pipe(2) and socket(2) more costly, for no visible reason.

Please see:
shmem_zero_setup()

    shmem_file_setup()
      __shmem_file_setup()
        alloc_file()

shmem_zero_setup() is used by /dev/zero (drivers/char/mem.c) and mmap(MAP_ANON). So yes, we do call mmap() on these files.

I also disagree on "for no visible reason". The reason to do this is uniformity. We make i_writecount work on all inodes regardless how they got created. Breaking consistent behavior just to save an atomic_inc_unless_negative() in the alloc_file() path seems unreasonable to me.

Anyhow, I haven't found any bug if we follow your recommendation. Every path using alloc_file() either prevents write access on the underlying inode (eg., anon-inode) or prevents user-space from getting an FD (all the shmem_file_setup() paths). So it's up to you. Feel free to fix it yourself, otherwise I will send a second patch following your idea tomorrow.

Quoted 6 lines · David Herrmann

Please see:
shmem_zero_setup()

    shmem_file_setup()
      __shmem_file_setup()
        alloc_file()

Quoted 2 lines

shmem_zero_setup() is used by /dev/zero (drivers/char/mem.c) and mmap(MAP_ANON). So yes, we do call mmap() on these files.

We do, but how do you get anything even attempt deny_write_access() on those? And what would the semantics of that be, anyway?

fs/file_table.c +4 -37

index 5b24008..ce1504f 100644
@@ -178,47 +177,12 @@ struct file *alloc_file(struct path *path, fmode_t mode,
        file->f_mapping = path->dentry->d_inode->i_mapping;
        file->f_mode = mode;
        file->f_op = fop;
-
-       /*
-        * These mounts don't really matter in practice
-        * for r/o bind mounts.  They aren't userspace-
-        * visible.  We do this for consistency, and so
-        * that we can do debugging checks at __fput()
-        */
-       if ((mode & FMODE_WRITE) && !special_file(path->dentry->d_inode->i_mode)) {
-               file_take_write(file);
-               WARN_ON(mnt_clone_write(path->mnt));
-       }
        if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
                i_readcount_inc(path->dentry->d_inode);
        return file;
 }
 EXPORT_SYMBOL(alloc_file);
 
-/**
- * drop_file_write_access - give up ability to write to a file
- * @file: the file to which we will stop writing
- *
- * This is a central place which will give up the ability
- * to write to @file, along with access to write through
- * its vfsmount.
- */
-static void drop_file_write_access(struct file *file)
-{
-       struct vfsmount *mnt = file->f_path.mnt;
-       struct dentry *dentry = file->f_path.dentry;
-       struct inode *inode = dentry->d_inode;
-
-       put_write_access(inode);
-
-       if (special_file(inode->i_mode))
-               return;
-       if (file_check_writeable(file) != 0)
-               return;
-       __mnt_drop_write(mnt);
-       file_release_write(file);
-}
-
 /* the real guts of fput() - releasing the last reference to file
  */
 static void __fput(struct file *file)

@@ -253,8 +217,10 @@ static void __fput(struct file *file)
        put_pid(file->f_owner.pid);
        if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
                i_readcount_dec(inode);
-       if (file->f_mode & FMODE_WRITE)
-               drop_file_write_access(file);
+       if (file->f_mode & FMODE_WRITER) {
+               put_write_access(inode);
+               __mnt_drop_write(mnt);
+       }
        file->f_path.dentry = NULL;
        file->f_path.mnt = NULL;
        file->f_inode = NULL;

fs/open.c +0 -32

index b9ed8b2..ea765e4 100644
@@ -632,35 +632,6 @@ out:
        return error;
 }
 
-/*
- * You have to be very careful that these write
- * counts get cleaned up in error cases and
- * upon __fput().  This should probably never
- * be called outside of __dentry_open().
- */
-static inline int __get_file_write_access(struct inode *inode,
-                                         struct vfsmount *mnt)
-{
-       int error;
-       error = get_write_access(inode);
-       if (error)
-               return error;
-       /*
-        * Do not take mount writer counts on
-        * special files since no writes to
-        * the mount itself will occur.
-        */
-       if (!special_file(inode->i_mode)) {
-               /*
-                * Balanced in __fput()
-                */
-               error = __mnt_want_write(mnt);
-               if (error)
-                       put_write_access(inode);
-       }
-       return error;
-}
-
 int open_check_o_direct(struct file *f)
 {
        /* NB: we're sure to have correct a_ops only after f_op->open */

@@ -690,12 +661,15 @@ static int do_dentry_open(struct file *f,
 
        path_get(&f->f_path);
        inode = f->f_inode = f->f_path.dentry->d_inode;
-       if (f->f_mode & FMODE_WRITE) {
-               error = __get_file_write_access(inode, f->f_path.mnt);
-               if (error)

From a first glance you could splitt of the removal of

We do, but how do you get anything even attempt deny_write_access() on those? And what would the semantics of that be, anyway?

Quoted 5 lines · Linus Torvalds

which returns ETXTBSY (most easily seen by just stracing it).

The patch would also seem to make sense, with the i_readcount_inc() being immediately below for the FMODE_READ case.

I think it's trying to fix the problem in the wrong place. The bug is real, all right, but it's not that alloc_file() for non-regulars doesn't grab writecount; it's that drop_file_write_access() drops it for those.

What the hell would we want to play with that counter for, anyway? It's not as if they could be mmapped, so all it does is making pipe(2) and socket(2) more costly, for no visible reason.

I would prefer to flip

        put_write_access(inode);

        if (special_file(inode->i_mode))
                return;

in drop_file_write_access() instead.

<goes to looks at i_writecount users>
Oh, shit...

drivers/md/md.c:
/* similar to deny_write_access, but accounts for our holding a reference
* to the file ourselves */
static int deny_bitmap_write_access(struct file * file)

{
        struct inode *inode = file->f_mapping->host;

        spin_lock(&inode->i_lock);
        if (atomic_read(&inode->i_writecount) > 1) {
                spin_unlock(&inode->i_lock);
                return -ETXTBSY;
        }
        atomic_set(&inode->i_writecount, -1);
        spin_unlock(&inode->i_lock);

        return 0;
}

Broken. get_write_access() will happily increment i_writecount e.g. from 1 to 2, without even looking at i_lock.

Moreover, it's paired with
void restore_bitmap_write_access(struct file *file)

{
        struct inode *inode = file->f_mapping->host;

        spin_lock(&inode->i_lock);
        atomic_set(&inode->i_writecount, 1);
        spin_unlock(&inode->i_lock);
}

Just what will happen if we do denywrite mmap() of that file in between? Even worse, the caller take file straight from fget(), with no sanity checks whatsoever. Just what will happen if I give it e.g. a directory? Or a procfs/sysfs/whatnot file, for that matter? Neil? I realize that it's root-only, but still...

drivers/md/md.c static int set_bitmap_file(struct mddev *mddev, int fd) · line 5986

Quoted 9 lines · NeilBrown

drivers/md/md.c +9 -0

+		inode = mddev->bitmap_info.file->f_mapping->host;
+		if (!S_ISREG(inode->i_mode)) {
+			printk(KERN_ERR "%s: error: bitmap file must be a regular file\n",
+			       mdname(mddev));
+			fput(mddev->bitmap_info.file);
+			mddev->bitmap_info.file = NULL;
+			return -EBADF;
+		}
+		if (atomic_read(&inode->i_writecount) != 1) {

Umm... I think you ought to check more than that. At the very least you want to check that you have it opened for write - you don't want e.g. a filesystem containing that puppy remounted r/o under you. Another thing is, what happens if it's not a buffer cache backed one? Hell, what happens if it's a file on NFS? You are relying on bmap() working, right? So it looks like you ought to check if ->bmap() is there. And I really wonder how well does it play with journalling fs...

Can we do direct writes from kernel space yet? If so I'll change the code to do that so that it will work with any filesystem (which supports direct writes).

(The documentation says we that bitmap files should only be used on ext2 or ext3. Most people use bitmaps on the raw devices so hopefully the few who have a need for files will read the documentation :-)

(and yes, I check for FMODE_WRITE)