| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20111108135514.GB9658@quack.suse.cz>
Date: Tue, 8 Nov 2011 14:55:14 +0100
From: Jan Kara <jack@...e.cz>
To: Aditya Kali <adityakali@...gle.com>
Cc: adilger@...ger.ca, tytso@....edu, dmonakhov@...nvz.org,
jack@...e.cz, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v3] ext4: make quota as first class supported feature
Thanks for the patch. I did a detailed review of it. I have quite some
comments but all of them are easily fixable so I think we should be mostly
ready in the next iteration.
On Thu 03-11-11 19:15:20, Aditya Kali wrote:
> +static const struct quotactl_ops ext4_new_qctl_operations = {
ext4_new_qctl_operations is rather poor name. What if you need to create
even newer set of operations in future? So I'd rather name them by "what
they do" - so something like ext4_sysfile_qctl_operations - meaning that these
operations handle quota stored in hidden system files.
> + .quota_on_meta = ext4_quota_on_meta,
> + .quota_off = ext4_quota_off,
> + .quota_sync = dquot_quota_sync,
> + .get_info = dquot_get_dqinfo,
> + .set_info = dquot_set_dqinfo,
> + .get_dqblk = dquot_get_dqblk,
> + .set_dqblk = dquot_set_dqblk
..
> @@ -3610,6 +3623,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> #ifdef CONFIG_QUOTA
> sb->s_qcop = &ext4_qctl_operations;
> sb->dq_op = &ext4_quota_operations;
> +
> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) {
> + /* Use new qctl operations with quota on function that does not
> + * require user specified quota file path. */
> + sb->s_qcop = &ext4_new_qctl_operations;
> +
> + sbi->s_qf_inums[USRQUOTA] = es->s_usr_quota_inum;
> + sbi->s_qf_inums[GRPQUOTA] = es->s_grp_quota_inum;
You need to convert from ondisk format here - i.e. use le32_to_cpu().
> + }
> #endif
> memcpy(sb->s_uuid, es->s_uuid, sizeof(es->s_uuid));
>
> @@ -3815,8 +3837,21 @@ no_journal:
> } else
> descr = "out journal";
>
> - ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> - "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> +#ifdef CONFIG_QUOTA
> + /* Enable quota usage during mount. */
> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) &&
> + !(sb->s_flags & MS_RDONLY)) {
> + ext4_quota_enable(sb, USRQUOTA, QFMT_VFS_V1,
> + DQUOT_USAGE_ENABLED);
> + ext4_quota_enable(sb, GRPQUOTA, QFMT_VFS_V1,
> + DQUOT_USAGE_ENABLED);
> + }
You should check the return value here and fail the mount in case turning
quotas on fails... Also you should set DQUOT_QUOTA_SYS_FILE in
sb_dqopt(sb)->flags before calling ext4_quota_enable() to indicate that
quota file is a hidden system file and generic quota code can then skip
some rather costly steps during quota handling to keep user visible and
kernel visible content in sync.
> +#endif /* CONFIG_QUOTA */
> +
> + ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. quota=%s. "
> + "Opts: %s%s%s", descr,
> + sb_any_quota_loaded(sb) ? "on" : "off",
> + sbi->s_es->s_mount_opts,
> *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
Should info about quotas really be in the message? After all it's just
another feature and we definitely don't report all the features filesystem
has set.
> @@ -4183,6 +4218,12 @@ static int ext4_commit_super(struct super_block *sb, int sync)
> es->s_free_inodes_count =
> cpu_to_le32(percpu_counter_sum_positive(
> &EXT4_SB(sb)->s_freeinodes_counter));
> +#ifdef CONFIG_QUOTA
> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) {
> + es->s_usr_quota_inum = EXT4_SB(sb)->s_qf_inums[USRQUOTA];
> + es->s_grp_quota_inum = EXT4_SB(sb)->s_qf_inums[GRPQUOTA];
> + }
> +#endif
Is there really need to reset these numbers? They should be stable
during the whole lifetime of a filesystem, no (modulo changes by tune2fs)?
If I'm missing something and numbers indeed can change, you should convert
them to on disk format - i.e. cpu_to_le32().
> @@ -4814,6 +4855,59 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
> return dquot_quota_on(sb, type, format_id, path);
> }
>
> +static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
> + unsigned int flags)
> +{
> + struct ext4_sb_info *sbi = EXT4_SB(sb);
> + struct inode *qf_inode;
> + int err;
> +
> + if (!sbi->s_qf_inums[type])
> + return -EPERM;
> +
> + qf_inode = ext4_iget(sb, sbi->s_qf_inums[type]);
> + if (IS_ERR(qf_inode)) {
> + ext4_error(sb, "Bad quota inode # %lu", sbi->s_qf_inums[type]);
> + return PTR_ERR(qf_inode);
> + }
> +
> + /*
> + * When we journal data on quota file, we have to flush journal to see
> + * all updates to the file when we bypass pagecache...
> + */
> + if (sbi->s_journal &&
> + ext4_should_journal_data(qf_inode)) {
> + /*
> + * We don't need to lock updates but journal_flush() could
> + * otherwise be livelocked...
> + */
> + jbd2_journal_lock_updates(sbi->s_journal);
> + err = jbd2_journal_flush(sbi->s_journal);
> + jbd2_journal_unlock_updates(sbi->s_journal);
> + if (err)
> + return err;
> + }
Why do you have the above check? Since this function is called only for
reserved quota inodes which are hidden, ext4_should_journal_data() will
never be true for them.
> +
> + err = dquot_enable(qf_inode, type, format_id, flags);
> + iput(qf_inode);
> +
> + return err;
> +}
> @@ -4837,7 +4931,12 @@ static int ext4_quota_off(struct super_block *sb, int type)
> ext4_journal_stop(handle);
>
> out:
> - return dquot_quota_off(sb, type);
> + /* Disable only the limits if QUOTA feature is set. */
> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA))
> + return dquot_disable(sb, type, DQUOT_LIMITS_ENABLED);
> + else
> + return dquot_disable(sb, type,
> + DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
> }
Hmm, most of the work ext4_quota_off() does is unnecessary when quota is
handled internally - there's no need to sync the filesystem, no need to
update mtime or ctime... So I'd rather provide a new ext4_quota_off()
function in case quota feature is enabled and that would just call
dquot_disable(sb, type, DQUOT_LIMITS_ENABLED).
Honza
--
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists