Re: [PATCH] fs: fix i_writecount on shmem and friends

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Mar 11, 2014 at 12:05:09PM -0700, Linus Torvalds wrote:
> 
> 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...