From 14e43bf435612639cab01541fce7cc41bf7e370b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 22 Sep 2020 09:44:18 -0700 Subject: vfs: don't unnecessarily clone write access for writable fds There's no need for mnt_want_write_file() to increment mnt_writers when the file is already open for writing, provided that mnt_drop_write_file() is changed to conditionally decrement it. We seem to have ended up in the current situation because mnt_want_write_file() used to be paired with mnt_drop_write(), due to mnt_drop_write_file() not having been added yet. So originally mnt_want_write_file() had to always increment mnt_writers. But later mnt_drop_write_file() was added, and all callers of mnt_want_write_file() were paired with it. This makes the compatibility between mnt_want_write_file() and mnt_drop_write() no longer necessary. Therefore, make __mnt_want_write_file() and __mnt_drop_write_file() skip incrementing mnt_writers on files already open for writing. This removes the only caller of mnt_clone_write(), so remove that too. Signed-off-by: Eric Biggers Signed-off-by: Al Viro --- Documentation/filesystems/porting.rst | 7 +++++ fs/namespace.c | 53 +++++++++++++---------------------- include/linux/mount.h | 1 - 3 files changed, 27 insertions(+), 34 deletions(-) diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst index 867036aa90b8..6a6d3e673b48 100644 --- a/Documentation/filesystems/porting.rst +++ b/Documentation/filesystems/porting.rst @@ -865,3 +865,10 @@ no matter what. Everything is handled by the caller. clone_private_mount() returns a longterm mount now, so the proper destructor of its result is kern_unmount() or kern_unmount_array(). + +--- + +**mandatory** + +mnt_want_write_file() can now only be paired with mnt_drop_write_file(), +whereas previously it could be paired with mnt_drop_write() as well. diff --git a/fs/namespace.c b/fs/namespace.c index d2db7dfe232b..9f2d94e0f3e0 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -359,51 +359,37 @@ int mnt_want_write(struct vfsmount *m) } EXPORT_SYMBOL_GPL(mnt_want_write); -/** - * mnt_clone_write - get write access to a mount - * @mnt: the mount on which to take a write - * - * This is effectively like mnt_want_write, except - * it must only be used to take an extra write reference - * on a mountpoint that we already know has a write reference - * on it. This allows some optimisation. - * - * After finished, mnt_drop_write must be called as usual to - * drop the reference. - */ -int mnt_clone_write(struct vfsmount *mnt) -{ - /* superblock may be r/o */ - if (__mnt_is_readonly(mnt)) - return -EROFS; - preempt_disable(); - mnt_inc_writers(real_mount(mnt)); - preempt_enable(); - return 0; -} -EXPORT_SYMBOL_GPL(mnt_clone_write); - /** * __mnt_want_write_file - get write access to a file's mount * @file: the file who's mount on which to take a write * - * This is like __mnt_want_write, but it takes a file and can - * do some optimisations if the file is open for write already + * This is like __mnt_want_write, but if the file is already open for writing it + * skips incrementing mnt_writers (since the open file already has a reference) + * and instead only does the check for emergency r/o remounts. This must be + * paired with __mnt_drop_write_file. */ int __mnt_want_write_file(struct file *file) { - if (!(file->f_mode & FMODE_WRITER)) - return __mnt_want_write(file->f_path.mnt); - else - return mnt_clone_write(file->f_path.mnt); + if (file->f_mode & FMODE_WRITER) { + /* + * Superblock may have become readonly while there are still + * writable fd's, e.g. due to a fs error with errors=remount-ro + */ + if (__mnt_is_readonly(file->f_path.mnt)) + return -EROFS; + return 0; + } + return __mnt_want_write(file->f_path.mnt); } /** * mnt_want_write_file - get write access to a file's mount * @file: the file who's mount on which to take a write * - * This is like mnt_want_write, but it takes a file and can - * do some optimisations if the file is open for write already + * This is like mnt_want_write, but if the file is already open for writing it + * skips incrementing mnt_writers (since the open file already has a reference) + * and instead only does the freeze protection and the check for emergency r/o + * remounts. This must be paired with mnt_drop_write_file. */ int mnt_want_write_file(struct file *file) { @@ -449,7 +435,8 @@ EXPORT_SYMBOL_GPL(mnt_drop_write); void __mnt_drop_write_file(struct file *file) { - __mnt_drop_write(file->f_path.mnt); + if (!(file->f_mode & FMODE_WRITER)) + __mnt_drop_write(file->f_path.mnt); } void mnt_drop_write_file(struct file *file) diff --git a/include/linux/mount.h b/include/linux/mount.h index aaf343b38671..b43191fe6af7 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -79,7 +79,6 @@ struct path; extern int mnt_want_write(struct vfsmount *mnt); extern int mnt_want_write_file(struct file *file); -extern int mnt_clone_write(struct vfsmount *mnt); extern void mnt_drop_write(struct vfsmount *mnt); extern void mnt_drop_write_file(struct file *file); extern void mntput(struct vfsmount *mnt); -- cgit v1.2.3 From edbb35cc6bdfc379a2968f17d479567650ddbb16 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 30 Oct 2020 17:44:20 -0700 Subject: fs/inode.c: make inode_init_always() initialize i_ino to 0 Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes. So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though. The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not. There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0. So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant. Signed-off-by: Eric Biggers Signed-off-by: Al Viro --- fs/inode.c | 1 + fs/jfs/super.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/inode.c b/fs/inode.c index 6442d97d9a4a..6598ea2bb097 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode) atomic_set(&inode->i_count, 1); inode->i_op = &empty_iops; inode->i_fop = &no_open_fops; + inode->i_ino = 0; inode->__i_nlink = 1; inode->i_opflags = 0; if (sb->s_xattr) diff --git a/fs/jfs/super.c b/fs/jfs/super.c index b2dc4d1f9dcc..1f0ffabbde56 100644 --- a/fs/jfs/super.c +++ b/fs/jfs/super.c @@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent) ret = -ENOMEM; goto out_unload; } - inode->i_ino = 0; inode->i_size = i_size_read(sb->s_bdev->bd_inode); inode->i_mapping->a_ops = &jfs_metapage_aops; inode_fake_hash(inode); -- cgit v1.2.3 From b1adbdbda458b2ec69bf5915c4dcdbe2bd5e7bad Mon Sep 17 00:00:00 2001 From: Al Viro Date: Fri, 29 Jan 2021 21:36:33 -0500 Subject: audit_alloc_mark(): don't open-code ERR_CAST() Signed-off-by: Al Viro --- kernel/audit_fsnotify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index 5b3f01da172b..60739d5e3373 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -84,7 +84,7 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa dentry = kern_path_locked(pathname, &path); if (IS_ERR(dentry)) - return (void *)dentry; /* returning an error */ + return ERR_CAST(dentry); /* returning an error */ inode = path.dentry->d_inode; inode_unlock(inode); -- cgit v1.2.3 From 9652c73246b980b9f2387916c35e02638d163472 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sun, 31 Jan 2021 14:40:40 -0500 Subject: 9p: fix misuse of sscanf() in v9fs_stat2inode() 1) sscanf() return value needs to be checked, damnit 2) sscanf() is perfectly capable of checking for fixed prefix, no need for that %13s + strncmp with constant string. 3) st->extension is a valid string; no need for voodoo with str*cpy() there. Signed-off-by: Al Viro --- fs/9p/vfs_inode.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 4a937fac1acb..58f6b56ef145 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -1137,9 +1137,6 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode, struct super_block *sb, unsigned int flags) { umode_t mode; - char ext[32]; - char tag_name[14]; - unsigned int i_nlink; struct v9fs_session_info *v9ses = sb->s_fs_info; struct v9fs_inode *v9inode = V9FS_I(inode); @@ -1157,18 +1154,18 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode, inode->i_gid = stat->n_gid; } if ((S_ISREG(inode->i_mode)) || (S_ISDIR(inode->i_mode))) { - if (v9fs_proto_dotu(v9ses) && (stat->extension[0] != '\0')) { + if (v9fs_proto_dotu(v9ses)) { + unsigned int i_nlink; /* - * Hadlink support got added later to - * to the .u extension. So there can be - * server out there that doesn't support - * this even with .u extension. So check - * for non NULL stat->extension + * Hadlink support got added later to the .u extension. + * So there can be a server out there that doesn't + * support this even with .u extension. That would + * just leave us with stat->extension being an empty + * string, though. */ - strlcpy(ext, stat->extension, sizeof(ext)); /* HARDLINKCOUNT %u */ - sscanf(ext, "%13s %u", tag_name, &i_nlink); - if (!strncmp(tag_name, "HARDLINKCOUNT", 13)) + if (sscanf(stat->extension, + " HARDLINKCOUNT %u", &i_nlink) == 1) set_nlink(inode, i_nlink); } } -- cgit v1.2.3 From 6f24784f00f2b5862b367caeecc5cca22a77faa3 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sun, 31 Jan 2021 19:23:55 -0500 Subject: whack-a-mole: don't open-code iminor/imajor several instances creeped back into the tree... Signed-off-by: Al Viro --- arch/sh/boards/mach-landisk/gio.c | 6 ++---- drivers/block/loop.c | 2 +- drivers/dax/super.c | 2 +- drivers/rtc/rtc-m41t80.c | 4 ++-- drivers/s390/char/vmur.c | 2 +- drivers/staging/vme/devices/vme_user.c | 12 ++++++------ fs/gfs2/inode.c | 4 ++-- 7 files changed, 15 insertions(+), 17 deletions(-) diff --git a/arch/sh/boards/mach-landisk/gio.c b/arch/sh/boards/mach-landisk/gio.c index 1c0da99dfc60..ff2200fec29a 100644 --- a/arch/sh/boards/mach-landisk/gio.c +++ b/arch/sh/boards/mach-landisk/gio.c @@ -27,11 +27,10 @@ static int openCnt; static int gio_open(struct inode *inode, struct file *filp) { - int minor; + int minor = iminor(inode); int ret = -ENOENT; preempt_disable(); - minor = MINOR(inode->i_rdev); if (minor < DEVCOUNT) { if (openCnt > 0) { ret = -EALREADY; @@ -46,9 +45,8 @@ static int gio_open(struct inode *inode, struct file *filp) static int gio_close(struct inode *inode, struct file *filp) { - int minor; + int minor = iminor(inode); - minor = MINOR(inode->i_rdev); if (minor < DEVCOUNT) { openCnt--; } diff --git a/drivers/block/loop.c b/drivers/block/loop.c index e5ff328f0917..b51330017ce1 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -663,7 +663,7 @@ static inline int is_loop_device(struct file *file) { struct inode *i = file->f_mapping->host; - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR; + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR; } static int loop_validate_file(struct file *file, struct block_device *bdev) diff --git a/drivers/dax/super.c b/drivers/dax/super.c index cadbd0a1a1ef..5fa6ae9dbc8b 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -480,7 +480,7 @@ static void dax_free_inode(struct inode *inode) kfree(dax_dev->host); dax_dev->host = NULL; if (inode->i_rdev) - ida_simple_remove(&dax_minor_ida, MINOR(inode->i_rdev)); + ida_simple_remove(&dax_minor_ida, iminor(inode)); kmem_cache_free(dax_cache, dax_dev); } diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c index 160dcf68e64e..1e5873261e7e 100644 --- a/drivers/rtc/rtc-m41t80.c +++ b/drivers/rtc/rtc-m41t80.c @@ -783,7 +783,7 @@ static long wdt_unlocked_ioctl(struct file *file, unsigned int cmd, */ static int wdt_open(struct inode *inode, struct file *file) { - if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) { + if (iminor(inode) == WATCHDOG_MINOR) { mutex_lock(&m41t80_rtc_mutex); if (test_and_set_bit(0, &wdt_is_open)) { mutex_unlock(&m41t80_rtc_mutex); @@ -807,7 +807,7 @@ static int wdt_open(struct inode *inode, struct file *file) */ static int wdt_release(struct inode *inode, struct file *file) { - if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) + if (iminor(inode) == WATCHDOG_MINOR) clear_bit(0, &wdt_is_open); return 0; } diff --git a/drivers/s390/char/vmur.c b/drivers/s390/char/vmur.c index 1bbf27b98cf6..68f49e2e964c 100644 --- a/drivers/s390/char/vmur.c +++ b/drivers/s390/char/vmur.c @@ -681,7 +681,7 @@ static int ur_open(struct inode *inode, struct file *file) * We treat the minor number as the devno of the ur device * to find in the driver tree. */ - devno = MINOR(file_inode(file)->i_rdev); + devno = iminor(file_inode(file)); urd = urdev_get_from_devno(devno); if (!urd) { diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c index fd0ea4dbcb91..568698fc3d3f 100644 --- a/drivers/staging/vme/devices/vme_user.c +++ b/drivers/staging/vme/devices/vme_user.c @@ -175,7 +175,7 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf, static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) { - unsigned int minor = MINOR(file_inode(file)->i_rdev); + unsigned int minor = iminor(file_inode(file)); ssize_t retval; size_t image_size; @@ -218,7 +218,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count, static ssize_t vme_user_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - unsigned int minor = MINOR(file_inode(file)->i_rdev); + unsigned int minor = iminor(file_inode(file)); ssize_t retval; size_t image_size; @@ -260,7 +260,7 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf, static loff_t vme_user_llseek(struct file *file, loff_t off, int whence) { - unsigned int minor = MINOR(file_inode(file)->i_rdev); + unsigned int minor = iminor(file_inode(file)); size_t image_size; loff_t res; @@ -294,7 +294,7 @@ static int vme_user_ioctl(struct inode *inode, struct file *file, struct vme_slave slave; struct vme_irq_id irq_req; unsigned long copied; - unsigned int minor = MINOR(inode->i_rdev); + unsigned int minor = iminor(inode); int retval; dma_addr_t pci_addr; void __user *argp = (void __user *)arg; @@ -412,7 +412,7 @@ vme_user_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { int ret; struct inode *inode = file_inode(file); - unsigned int minor = MINOR(inode->i_rdev); + unsigned int minor = iminor(inode); mutex_lock(&image[minor].mutex); ret = vme_user_ioctl(inode, file, cmd, arg); @@ -481,7 +481,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma) static int vme_user_mmap(struct file *file, struct vm_area_struct *vma) { - unsigned int minor = MINOR(file_inode(file)->i_rdev); + unsigned int minor = iminor(file_inode(file)); if (type[minor] == MASTER_MINOR) return vme_user_master_mmap(minor, vma); diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index c1b77e8d6b1c..6cabe5bba1c8 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -490,8 +490,8 @@ static void init_dinode(struct gfs2_inode *dip, struct gfs2_inode *ip, di = (struct gfs2_dinode *)dibh->b_data; gfs2_dinode_out(ip, di); - di->di_major = cpu_to_be32(MAJOR(ip->i_inode.i_rdev)); - di->di_minor = cpu_to_be32(MINOR(ip->i_inode.i_rdev)); + di->di_major = cpu_to_be32(imajor(&ip->i_inode)); + di->di_minor = cpu_to_be32(iminor(&ip->i_inode)); di->__pad1 = 0; di->__pad2 = 0; di->__pad3 = 0; -- cgit v1.2.3