From 7d6cd73d33b62021111a469b6a454ec357be295f Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 23 Nov 2016 13:16:10 +0100 Subject: quota: Hold s_umount in exclusive mode when enabling / disabling quotas Currently we hold s_umount semaphore only in shared mode when enabling or disabling quotas and use dqonoff_mutex for serializing quota state changes on a filesystem and also quota state changes with other places depending on current quota state. Using dedicated mutex for this causes possible deadlocks during filesystem freezing (see following commit for details) so we transition to using s_umount semaphore for the necessary synchronization whose lock ordering is properly handled by the filesystem freezing code. As a start grab s_umount in exclusive mode when enabling / disabling quotas. Signed-off-by: Jan Kara --- fs/quota/dquot.c | 11 +++++++++++ fs/quota/quota.c | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) (limited to 'fs/quota') diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 1bfac28b7e7d..047afb966420 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -2107,6 +2107,10 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) struct quota_info *dqopt = sb_dqopt(sb); struct inode *toputinode[MAXQUOTAS]; + /* s_umount should be held in exclusive mode */ + if (WARN_ON_ONCE(down_read_trylock(&sb->s_umount))) + up_read(&sb->s_umount); + /* Cannot turn off usage accounting without turning off limits, or * suspend quotas and simultaneously turn quotas off. */ if ((flags & DQUOT_USAGE_ENABLED && !(flags & DQUOT_LIMITS_ENABLED)) @@ -2371,6 +2375,10 @@ int dquot_resume(struct super_block *sb, int type) int ret = 0, cnt; unsigned int flags; + /* s_umount should be held in exclusive mode */ + if (WARN_ON_ONCE(down_read_trylock(&sb->s_umount))) + up_read(&sb->s_umount); + for (cnt = 0; cnt < MAXQUOTAS; cnt++) { if (type != -1 && cnt != type) continue; @@ -2430,6 +2438,9 @@ int dquot_enable(struct inode *inode, int type, int format_id, /* Just unsuspend quotas? */ BUG_ON(flags & DQUOT_SUSPENDED); + /* s_umount should be held in exclusive mode */ + if (WARN_ON_ONCE(down_read_trylock(&sb->s_umount))) + up_read(&sb->s_umount); if (!flags) return 0; diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 2d445425aad7..6ce6f4b6826b 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -789,9 +789,14 @@ static int quotactl_cmd_write(int cmd) } return 1; } - #endif /* CONFIG_BLOCK */ +/* Return true if quotactl command is manipulating quota on/off state */ +static bool quotactl_cmd_onoff(int cmd) +{ + return (cmd == Q_QUOTAON) || (cmd == Q_QUOTAOFF); +} + /* * look up a superblock on which quota ops will be performed * - use the name of a block device to find the superblock thereon @@ -809,7 +814,9 @@ static struct super_block *quotactl_block(const char __user *special, int cmd) putname(tmp); if (IS_ERR(bdev)) return ERR_CAST(bdev); - if (quotactl_cmd_write(cmd)) + if (quotactl_cmd_onoff(cmd)) + sb = get_super_exclusive_thawed(bdev); + else if (quotactl_cmd_write(cmd)) sb = get_super_thawed(bdev); else sb = get_super(bdev); @@ -872,7 +879,10 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, ret = do_quotactl(sb, type, cmds, id, addr, pathp); - drop_super(sb); + if (!quotactl_cmd_onoff(cmds)) + drop_super(sb); + else + drop_super_exclusive(sb); out: if (pathp && !IS_ERR(pathp)) path_put(pathp); -- cgit v1.2.3 From 9d1ccbe70e0b14545caad12dc73adb3605447df0 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 23 Nov 2016 13:35:14 +0100 Subject: quota: Use s_umount protection for quota operations Writeback quota is protected by s_umount semaphore held for reading because every writeback must be protected by that lock (grabbed either by the generic writeback code or by quotactl handler). Getting next available ID in quota file, querying quota state, setting quota information, getting quota format are all quotactl operations protected by s_umount semaphore held for reading grabbed in quotactl handler. This also fixes lockdep splat about possible deadlock during filesystem freezing where sync_filesystem() is called with page-faults already blocked but sync_filesystem() calls into dquot_writeback_dquots() which grabs dqonoff_mutex which ranks above i_mutex (vfs_load_quota_inode() grabs i_mutex under dqonoff_mutex) which clearly ranks below page fault freeze protection (e.g. via mmap_sem dependencies). The reported problem is not a real deadlock possibility since during quota on we check whether filesystem freezing is not in progress but still it is good to have this fixed. Reported-by: Ted Tso Reported-by: Eric Whitney Signed-off-by: Jan Kara --- fs/quota/dquot.c | 39 ++++++++++----------------------------- fs/quota/quota.c | 6 +----- 2 files changed, 11 insertions(+), 34 deletions(-) (limited to 'fs/quota') diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 047afb966420..2a9dc3fb491c 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -617,7 +617,8 @@ int dquot_writeback_dquots(struct super_block *sb, int type) int cnt; int err, ret = 0; - mutex_lock(&dqopt->dqonoff_mutex); + WARN_ON_ONCE(!rwsem_is_locked(&sb->s_umount)); + for (cnt = 0; cnt < MAXQUOTAS; cnt++) { if (type != -1 && cnt != type) continue; @@ -653,7 +654,6 @@ int dquot_writeback_dquots(struct super_block *sb, int type) && info_dirty(&dqopt->info[cnt])) sb->dq_op->write_info(sb, cnt); dqstats_inc(DQST_SYNCS); - mutex_unlock(&dqopt->dqonoff_mutex); return ret; } @@ -683,7 +683,6 @@ int dquot_quota_sync(struct super_block *sb, int type) * Now when everything is written we can discard the pagecache so * that userspace sees the changes. */ - mutex_lock(&dqopt->dqonoff_mutex); for (cnt = 0; cnt < MAXQUOTAS; cnt++) { if (type != -1 && cnt != type) continue; @@ -693,7 +692,6 @@ int dquot_quota_sync(struct super_block *sb, int type) truncate_inode_pages(&dqopt->files[cnt]->i_data, 0); inode_unlock(dqopt->files[cnt]); } - mutex_unlock(&dqopt->dqonoff_mutex); return 0; } @@ -2050,21 +2048,13 @@ int dquot_get_next_id(struct super_block *sb, struct kqid *qid) struct quota_info *dqopt = sb_dqopt(sb); int err; - mutex_lock(&dqopt->dqonoff_mutex); - if (!sb_has_quota_active(sb, qid->type)) { - err = -ESRCH; - goto out; - } - if (!dqopt->ops[qid->type]->get_next_id) { - err = -ENOSYS; - goto out; - } + if (!sb_has_quota_active(sb, qid->type)) + return -ESRCH; + if (!dqopt->ops[qid->type]->get_next_id) + return -ENOSYS; mutex_lock(&dqopt->dqio_mutex); err = dqopt->ops[qid->type]->get_next_id(sb, qid); mutex_unlock(&dqopt->dqio_mutex); -out: - mutex_unlock(&dqopt->dqonoff_mutex); - return err; } EXPORT_SYMBOL(dquot_get_next_id); @@ -2762,7 +2752,6 @@ int dquot_get_state(struct super_block *sb, struct qc_state *state) struct quota_info *dqopt = sb_dqopt(sb); int type; - mutex_lock(&sb_dqopt(sb)->dqonoff_mutex); memset(state, 0, sizeof(*state)); for (type = 0; type < MAXQUOTAS; type++) { if (!sb_has_quota_active(sb, type)) @@ -2784,7 +2773,6 @@ int dquot_get_state(struct super_block *sb, struct qc_state *state) tstate->nextents = 1; /* We don't know... */ spin_unlock(&dq_data_lock); } - mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex); return 0; } EXPORT_SYMBOL(dquot_get_state); @@ -2798,18 +2786,13 @@ int dquot_set_dqinfo(struct super_block *sb, int type, struct qc_info *ii) if ((ii->i_fieldmask & QC_WARNS_MASK) || (ii->i_fieldmask & QC_RT_SPC_TIMER)) return -EINVAL; - mutex_lock(&sb_dqopt(sb)->dqonoff_mutex); - if (!sb_has_quota_active(sb, type)) { - err = -ESRCH; - goto out; - } + if (!sb_has_quota_active(sb, type)) + return -ESRCH; mi = sb_dqopt(sb)->info + type; if (ii->i_fieldmask & QC_FLAGS) { if ((ii->i_flags & QCI_ROOT_SQUASH && - mi->dqi_format->qf_fmt_id != QFMT_VFS_OLD)) { - err = -EINVAL; - goto out; - } + mi->dqi_format->qf_fmt_id != QFMT_VFS_OLD)) + return -EINVAL; } spin_lock(&dq_data_lock); if (ii->i_fieldmask & QC_SPC_TIMER) @@ -2826,8 +2809,6 @@ int dquot_set_dqinfo(struct super_block *sb, int type, struct qc_info *ii) mark_info_dirty(sb, type); /* Force write to disk */ sb->dq_op->write_info(sb, type); -out: - mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex); return err; } EXPORT_SYMBOL(dquot_set_dqinfo); diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 6ce6f4b6826b..413c36cca462 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -104,13 +104,9 @@ static int quota_getfmt(struct super_block *sb, int type, void __user *addr) { __u32 fmt; - mutex_lock(&sb_dqopt(sb)->dqonoff_mutex); - if (!sb_has_quota_active(sb, type)) { - mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex); + if (!sb_has_quota_active(sb, type)) return -ESRCH; - } fmt = sb_dqopt(sb)->info[type].dqi_format->qf_fmt_id; - mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex); if (copy_to_user(addr, &fmt, sizeof(fmt))) return -EFAULT; return 0; -- cgit v1.2.3 From ee1ac541a2446b13b2525929e719e84b63d5e5da Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 23 Nov 2016 13:52:19 +0100 Subject: quota: Remove dqonoff_mutex from dquot_scan_active() All callers of dquot_scan_active() now hold s_umount so we can rely on that lock to protect us against quota state changes. Signed-off-by: Jan Kara --- fs/quota/dquot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/quota') diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 2a9dc3fb491c..d91aecc939c9 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -572,7 +572,8 @@ int dquot_scan_active(struct super_block *sb, struct dquot *dquot, *old_dquot = NULL; int ret = 0; - mutex_lock(&sb_dqopt(sb)->dqonoff_mutex); + WARN_ON_ONCE(!rwsem_is_locked(&sb->s_umount)); + spin_lock(&dq_list_lock); list_for_each_entry(dquot, &inuse_list, dq_inuse) { if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags)) @@ -603,7 +604,6 @@ int dquot_scan_active(struct super_block *sb, spin_unlock(&dq_list_lock); out: dqput(old_dquot); - mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex); return ret; } EXPORT_SYMBOL(dquot_scan_active); -- cgit v1.2.3 From c3b004460d77bf3f980d877be539016f2df4df12 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 23 Nov 2016 14:04:55 +0100 Subject: quota: Remove dqonoff_mutex The only places that were grabbing dqonoff_mutex are functions turning quotas on and off and these are properly serialized using s_umount semaphore. Remove dqonoff_mutex. Signed-off-by: Jan Kara --- fs/quota/dquot.c | 80 +++++++++++++++------------------------------------ fs/super.c | 1 - include/linux/quota.h | 1 - 3 files changed, 23 insertions(+), 59 deletions(-) (limited to 'fs/quota') diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index d91aecc939c9..550f78f2b7c7 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -119,8 +119,7 @@ * spinlock to internal buffers before writing. * * Lock ordering (including related VFS locks) is the following: - * dqonoff_mutex > i_mutex > journal_lock > dquot->dq_lock > dqio_mutex - * dqonoff_mutex > i_mutex comes from dquot_quota_sync, dquot_enable, etc. + * s_umount > i_mutex > journal_lock > dquot->dq_lock > dqio_mutex */ static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_list_lock); @@ -933,7 +932,7 @@ static int dqinit_needed(struct inode *inode, int type) return 0; } -/* This routine is guarded by dqonoff_mutex mutex */ +/* This routine is guarded by s_umount semaphore */ static void add_dquot_ref(struct super_block *sb, int type) { struct inode *inode, *old_inode = NULL; @@ -2108,18 +2107,14 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) DQUOT_USAGE_ENABLED))) return -EINVAL; - /* We need to serialize quota_off() for device */ - mutex_lock(&dqopt->dqonoff_mutex); - /* * Skip everything if there's nothing to do. We have to do this because * sometimes we are called when fill_super() failed and calling * sync_fs() in such cases does no good. */ - if (!sb_any_quota_loaded(sb)) { - mutex_unlock(&dqopt->dqonoff_mutex); + if (!sb_any_quota_loaded(sb)) return 0; - } + for (cnt = 0; cnt < MAXQUOTAS; cnt++) { toputinode[cnt] = NULL; if (type != -1 && cnt != type) @@ -2173,7 +2168,6 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) dqopt->info[cnt].dqi_bgrace = 0; dqopt->ops[cnt] = NULL; } - mutex_unlock(&dqopt->dqonoff_mutex); /* Skip syncing and setting flags if quota files are hidden */ if (dqopt->flags & DQUOT_QUOTA_SYS_FILE) @@ -2191,19 +2185,13 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) * changes done by userspace on the next quotaon() */ for (cnt = 0; cnt < MAXQUOTAS; cnt++) if (toputinode[cnt]) { - mutex_lock(&dqopt->dqonoff_mutex); - /* If quota was reenabled in the meantime, we have - * nothing to do */ - if (!sb_has_quota_loaded(sb, cnt)) { - inode_lock(toputinode[cnt]); - toputinode[cnt]->i_flags &= ~(S_IMMUTABLE | + WARN_ON_ONCE(sb_has_quota_loaded(sb, cnt)); + inode_lock(toputinode[cnt]); + toputinode[cnt]->i_flags &= ~(S_IMMUTABLE | S_NOATIME | S_NOQUOTA); - truncate_inode_pages(&toputinode[cnt]->i_data, - 0); - inode_unlock(toputinode[cnt]); - mark_inode_dirty_sync(toputinode[cnt]); - } - mutex_unlock(&dqopt->dqonoff_mutex); + truncate_inode_pages(&toputinode[cnt]->i_data, 0); + inode_unlock(toputinode[cnt]); + mark_inode_dirty_sync(toputinode[cnt]); } if (sb->s_bdev) invalidate_bdev(sb->s_bdev); @@ -2275,6 +2263,10 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id, error = -EINVAL; goto out_fmt; } + if (sb_has_quota_loaded(sb, type)) { + error = -EBUSY; + goto out_fmt; + } if (!(dqopt->flags & DQUOT_QUOTA_SYS_FILE)) { /* As we bypass the pagecache we must now flush all the @@ -2286,11 +2278,6 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id, sync_filesystem(sb); invalidate_bdev(sb->s_bdev); } - mutex_lock(&dqopt->dqonoff_mutex); - if (sb_has_quota_loaded(sb, type)) { - error = -EBUSY; - goto out_lock; - } if (!(dqopt->flags & DQUOT_QUOTA_SYS_FILE)) { /* We don't want quota and atime on quota files (deadlocks @@ -2311,7 +2298,7 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id, error = -EIO; dqopt->files[type] = igrab(inode); if (!dqopt->files[type]) - goto out_lock; + goto out_file_flags; error = -EINVAL; if (!fmt->qf_ops->check_quota_file(sb, type)) goto out_file_init; @@ -2334,14 +2321,13 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id, spin_unlock(&dq_state_lock); add_dquot_ref(sb, type); - mutex_unlock(&dqopt->dqonoff_mutex); return 0; out_file_init: dqopt->files[type] = NULL; iput(inode); -out_lock: +out_file_flags: if (oldflags != -1) { inode_lock(inode); /* Set the flags back (in the case of accidental quotaon() @@ -2350,7 +2336,6 @@ out_lock: inode->i_flags |= oldflags; inode_unlock(inode); } - mutex_unlock(&dqopt->dqonoff_mutex); out_fmt: put_quota_format(fmt); @@ -2372,12 +2357,9 @@ int dquot_resume(struct super_block *sb, int type) for (cnt = 0; cnt < MAXQUOTAS; cnt++) { if (type != -1 && cnt != type) continue; - - mutex_lock(&dqopt->dqonoff_mutex); - if (!sb_has_quota_suspended(sb, cnt)) { - mutex_unlock(&dqopt->dqonoff_mutex); + if (!sb_has_quota_suspended(sb, cnt)) continue; - } + inode = dqopt->files[cnt]; dqopt->files[cnt] = NULL; spin_lock(&dq_state_lock); @@ -2386,7 +2368,6 @@ int dquot_resume(struct super_block *sb, int type) cnt); dqopt->flags &= ~dquot_state_flag(DQUOT_STATE_FLAGS, cnt); spin_unlock(&dq_state_lock); - mutex_unlock(&dqopt->dqonoff_mutex); flags = dquot_generic_flag(flags, cnt); ret = vfs_load_quota_inode(inode, cnt, @@ -2422,9 +2403,7 @@ EXPORT_SYMBOL(dquot_quota_on); int dquot_enable(struct inode *inode, int type, int format_id, unsigned int flags) { - int ret = 0; struct super_block *sb = inode->i_sb; - struct quota_info *dqopt = sb_dqopt(sb); /* Just unsuspend quotas? */ BUG_ON(flags & DQUOT_SUSPENDED); @@ -2436,31 +2415,18 @@ int dquot_enable(struct inode *inode, int type, int format_id, return 0; /* Just updating flags needed? */ if (sb_has_quota_loaded(sb, type)) { - mutex_lock(&dqopt->dqonoff_mutex); - /* Now do a reliable test... */ - if (!sb_has_quota_loaded(sb, type)) { - mutex_unlock(&dqopt->dqonoff_mutex); - goto load_quota; - } if (flags & DQUOT_USAGE_ENABLED && - sb_has_quota_usage_enabled(sb, type)) { - ret = -EBUSY; - goto out_lock; - } + sb_has_quota_usage_enabled(sb, type)) + return -EBUSY; if (flags & DQUOT_LIMITS_ENABLED && - sb_has_quota_limits_enabled(sb, type)) { - ret = -EBUSY; - goto out_lock; - } + sb_has_quota_limits_enabled(sb, type)) + return -EBUSY; spin_lock(&dq_state_lock); sb_dqopt(sb)->flags |= dquot_state_flag(flags, type); spin_unlock(&dq_state_lock); -out_lock: - mutex_unlock(&dqopt->dqonoff_mutex); - return ret; + return 0; } -load_quota: return vfs_load_quota_inode(inode, type, format_id, flags); } EXPORT_SYMBOL(dquot_enable); diff --git a/fs/super.c b/fs/super.c index f7f724230e2b..1709ed029a2c 100644 --- a/fs/super.c +++ b/fs/super.c @@ -244,7 +244,6 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, mutex_init(&s->s_vfs_rename_mutex); lockdep_set_class(&s->s_vfs_rename_mutex, &type->s_vfs_rename_key); mutex_init(&s->s_dquot.dqio_mutex); - mutex_init(&s->s_dquot.dqonoff_mutex); s->s_maxbytes = MAX_NON_LFS; s->s_op = &default_op; s->s_time_gran = 1000000000; diff --git a/include/linux/quota.h b/include/linux/quota.h index 55107a8ff887..b281d198ee5b 100644 --- a/include/linux/quota.h +++ b/include/linux/quota.h @@ -520,7 +520,6 @@ static inline void quota_send_warning(struct kqid qid, dev_t dev, struct quota_info { unsigned int flags; /* Flags for diskquotas on this device */ struct mutex dqio_mutex; /* lock device while I/O in progress */ - struct mutex dqonoff_mutex; /* Serialize quotaon & quotaoff */ struct inode *files[MAXQUOTAS]; /* inodes of quotafiles */ struct mem_dqinfo info[MAXQUOTAS]; /* Information for each quota type */ const struct quota_format_ops *ops[MAXQUOTAS]; /* Operations for each type */ -- cgit v1.2.3 From 2700e6067c72a99d1b7037692da0145ac44623c4 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 19 Dec 2016 14:01:39 +0100 Subject: quota: Fix bogus warning in dquot_disable() dquot_disable() was warning when sb_has_quota_loaded() was true when invalidating page cache for quota files. The thinking behind this warning was that we must have raced with somebody else turning quotas on and this should not happen because all places modifying quota state must hold s_umount exclusively now. However sb_has_quota_loaded() can be also true at this point when we are just suspending quotas on remount read-only. Just restore the behavior to situation before commit c3b004460d77 ("quota: Remove dqonoff_mutex") which introduced the warning. The code in dquot_disable() can be further simplified with the new locking of quota state changes however let's leave that to a separate commit that can get more testing exposure. Fixes: c3b004460d77bf3f980d877be539016f2df4df12 Signed-off-by: Jan Kara --- fs/quota/dquot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/quota') diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 550f78f2b7c7..1ed5494aa773 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -2184,8 +2184,8 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) * must also discard the blockdev buffers so that we see the * changes done by userspace on the next quotaon() */ for (cnt = 0; cnt < MAXQUOTAS; cnt++) - if (toputinode[cnt]) { - WARN_ON_ONCE(sb_has_quota_loaded(sb, cnt)); + /* This can happen when suspending quotas on remount-ro... */ + if (toputinode[cnt] && !sb_has_quota_loaded(sb, cnt)) { inode_lock(toputinode[cnt]); toputinode[cnt]->i_flags &= ~(S_IMMUTABLE | S_NOATIME | S_NOQUOTA); -- cgit v1.2.3