| 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: <YLl2kEZEDuIjrlfO@sol.localdomain>
Date: Thu, 3 Jun 2021 17:40:48 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Jaegeuk Kim <jaegeuk@...nel.org>
Cc: linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: clean up /sys/fs/f2fs/<disk>/features
On Thu, Jun 03, 2021 at 03:08:34PM -0700, Jaegeuk Kim wrote:
> Let's create /sys/fs/f2fs/<disk>/feature_list/ to meet sysfs rule.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> ---
> Documentation/ABI/testing/sysfs-fs-f2fs | 18 ++-
> fs/f2fs/f2fs.h | 3 +
> fs/f2fs/sysfs.c | 152 +++++++++++++++++++++++-
> 3 files changed, 168 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
> index 5088281e312e..43b2cde80b70 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -203,7 +203,23 @@ Description: Shows total written kbytes issued to disk.
> What: /sys/fs/f2fs/<disk>/features
> Date: July 2017
> Contact: "Jaegeuk Kim" <jaegeuk@...nel.org>
> -Description: Shows all enabled features in current device.
> +Description: <deprecated: should use /sys/fs/f2fs/<disk>/feature_list/
> + Shows all enabled features in current device.
> + Supported features:
> + encryption, blkzoned, extra_attr, projquota, inode_checksum,
> + flexible_inline_xattr, quota_ino, inode_crtime, lost_found,
> + verity, sb_checksum, casefold, readonly, compression,
> + encrypted_casefold, pin_file.
Isn't pin_file a feature of the implementation, not of a particular filesystem
instance? I.e. something that should go in /sys/fs/f2fs/features/, not here.
Likewise for encrypted_casefold, as it is implied by encryption && casefold.
> +
> +What: /sys/fs/f2fs/<disk>/feature_list/
> +Date: June 2021
> +Contact: "Jaegeuk Kim" <jaegeuk@...nel.org>
> +Description: Expand /sys/fs/f2fs/<disk>/features to meet sysfs rule.
> + Supported features:
> + encryption, block_zoned, extra_attr, projquota, inode_checksum,
> + flexible_inline_xattr, quota_ino, inode_crtime, lost_found,
> + verity, sb_checksum, casefold, readonly, compression,
> + encrypted_casefold, pin_file.
Is it intentional that the file has "blkzoned" but the directory has
"blk_zoned"?
Also, your code has another difference -- "project_quota" is used instead of
"projquota". But that's not mentioned above.
And encrypted_casefold and pin_file don't seem appropriate to include here, as
mentioned above.
> @@ -1665,6 +1665,9 @@ struct f2fs_sb_info {
> struct kobject s_stat_kobj; /* /sys/fs/f2fs/<devname>/stat */
> struct completion s_stat_kobj_unregister;
>
> + struct kobject s_disk_feat_kobj; /* /sys/fs/f2fs/<devname>/feature_list */
> + struct completion s_disk_feat_kobj_unregister;
This is for a particular filesystem instance, not a disk per se. (A f2fs
filesystem can use multiple disks.) So having "disk" in the name doesn't make
sense.
Please use more logical names like s_feature_list_kobj,
f2fs_sb_feature_list_ktype, f2fs_sb_feat_attrs, etc.
> static ssize_t f2fs_feature_show(struct f2fs_attr *a,
> struct f2fs_sb_info *sbi, char *buf)
> {
> + unsigned long feat_supp = 0;
> +
> switch (a->id) {
> case FEAT_CRYPTO:
> + feat_supp |= f2fs_sb_has_encrypt(sbi) ?
> + (1 << FEAT_CRYPTO) : 0;
> + fallthrough;
> case FEAT_BLKZONED:
> - case FEAT_ATOMIC_WRITE:
> + feat_supp |= f2fs_sb_has_blkzoned(sbi) ?
> + (1 << FEAT_BLKZONED) : 0;
> + fallthrough;
> case FEAT_EXTRA_ATTR:
> + feat_supp |= f2fs_sb_has_extra_attr(sbi) ?
> + (1 << FEAT_EXTRA_ATTR) : 0;
> + fallthrough;
> case FEAT_PROJECT_QUOTA:
> + feat_supp |= f2fs_sb_has_project_quota(sbi) ?
> + (1 << FEAT_PROJECT_QUOTA) : 0;
> + fallthrough;
> case FEAT_INODE_CHECKSUM:
> + feat_supp |= f2fs_sb_has_inode_chksum(sbi) ?
> + (1 << FEAT_INODE_CHECKSUM) : 0;
> + fallthrough;
> case FEAT_FLEXIBLE_INLINE_XATTR:
> + feat_supp |= f2fs_sb_has_flexible_inline_xattr(sbi) ?
> + (1 << FEAT_FLEXIBLE_INLINE_XATTR) : 0;
> + fallthrough;
> case FEAT_QUOTA_INO:
> + feat_supp |= f2fs_sb_has_quota_ino(sbi) ?
> + (1 << FEAT_QUOTA_INO) : 0;
> + fallthrough;
> case FEAT_INODE_CRTIME:
> + feat_supp |= f2fs_sb_has_inode_crtime(sbi) ?
> + (1 << FEAT_INODE_CRTIME) : 0;
> + fallthrough;
> case FEAT_LOST_FOUND:
> + feat_supp |= f2fs_sb_has_lost_found(sbi) ?
> + (1 << FEAT_LOST_FOUND) : 0;
> + fallthrough;
> case FEAT_VERITY:
> + feat_supp |= f2fs_sb_has_verity(sbi) ?
> + (1 << FEAT_VERITY) : 0;
> + fallthrough;
> case FEAT_SB_CHECKSUM:
> + feat_supp |= f2fs_sb_has_sb_chksum(sbi) ?
> + (1 << FEAT_SB_CHECKSUM) : 0;
> + fallthrough;
> case FEAT_CASEFOLD:
> + feat_supp |= f2fs_sb_has_casefold(sbi) ?
> + (1 << FEAT_CASEFOLD) : 0;
> + fallthrough;
> case FEAT_COMPRESSION:
> + feat_supp |= f2fs_sb_has_compression(sbi) ?
> + (1 << FEAT_COMPRESSION) : 0;
> + fallthrough;
> case FEAT_RO:
> - case FEAT_TEST_DUMMY_ENCRYPTION_V2:
> + feat_supp |= f2fs_sb_has_readonly(sbi) ?
> + (1 << FEAT_RO) : 0;
> + fallthrough;
> case FEAT_ENCRYPTED_CASEFOLD:
> - return sprintf(buf, "supported\n");
> + feat_supp |= (f2fs_sb_has_casefold(sbi) &&
> + f2fs_sb_has_encrypt(sbi)) ?
> + (1 << FEAT_ENCRYPTED_CASEFOLD) : 0;
> + fallthrough;
> + case FEAT_PIN_FILE:
> + feat_supp |= (1 << FEAT_PIN_FILE);
> + fallthrough;
> + case FEAT_TEST_DUMMY_ENCRYPTION_V2:
> + case FEAT_ATOMIC_WRITE:
> + if (!a->offset || feat_supp & (1 << a->id))
> + return sprintf(buf, "supported\n");
> }
> - return 0;
> + return sprintf(buf, "not supported\n");
> }
This function doesn't make much sense.
Part of the problem is that the same function is handling both
/sys/fs/f2fs/features/ and /sys/fs/f2fs/$s_id/feature_list/.
All the former needs is to print "supported", since unsupported is indicated by
the file not being there at all. So it should simply have its own ->show
function separate from the one for feature_list/.
And the feature_list/ ones could just store the F2FS_FEATURE_* bit in
f2fs_attr::id and check for it using F2FS_HAS_FEATURE(). That would be much
simpler -- no need for the feat_id enum or the long switch statement.
Also for feature_list/ it might be better to use "unsupported" than
"not supported", so that \<supported\> doesn't match...
> static struct f2fs_attr f2fs_attr_##_name = { \
> .attr = {.name = __stringify(_name), .mode = 0444 }, \
> .show = f2fs_feature_show, \
> + .offset = 0, \
> .id = _id, \
> }
There's no need to use the .offset argument if features/ and $s_id/feature_list/
just used different ->show functions.
> +#define F2FS_DISK_FEATURE_RO_ATTR(_name, _id) \
F2FS_SB_FEATURE_ATTR would be a much better name, since these pertain to a
filesystem instance (not necessarily a disk), and all the feature attributes are
read-only.
> static int __maybe_unused segment_info_seq_show(struct seq_file *seq,
> void *offset)
> {
> @@ -1149,6 +1279,15 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
> if (err)
> goto put_stat_kobj;
>
> + sbi->s_disk_feat_kobj.kset = &f2fs_kset;
> + init_completion(&sbi->s_disk_feat_kobj_unregister);
> + err = kobject_init_and_add(&sbi->s_disk_feat_kobj,
> + &f2fs_disk_feat_ktype,
> + &sbi->s_kobj, "feature_list");
> + if (err)
> + goto put_stat_kobj;
> +
> +
> if (f2fs_proc_root)
> sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
>
> @@ -1166,6 +1305,8 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
> put_stat_kobj:
> kobject_put(&sbi->s_stat_kobj);
> wait_for_completion(&sbi->s_stat_kobj_unregister);
> + kobject_put(&sbi->s_disk_feat_kobj);
> + wait_for_completion(&sbi->s_disk_feat_kobj_unregister);
It seems this should go to its own label.
Also, please note that it's very easy to get confused between
/sys/fs/f2fs/features/, /sys/fs/f2fs/$s_id/features, and
/sys/fs/f2fs/$s_id/feature_list/. Adding some comments could clarify things a
lot.
- Eric
Powered by blists - more mailing lists