Darrick,
On 2019/08/21 0:26, Darrick J. Wong wrote:
[...]
>> +/*
>> + * Read super block information from the device.
>> + */
>> +static int zonefs_read_super(struct super_block *sb)
>> +{
>> + struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>> + struct zonefs_super *super;
>> + struct bio bio;
>> + struct bio_vec bio_vec;
>> + struct page *page;
>> + int ret;
>> +
>> + page = alloc_page(GFP_KERNEL);
>> + if (!page)
>> + return -ENOMEM;
>> +
>> + bio_init(&bio, &bio_vec, 1);
>> + bio.bi_iter.bi_sector = 0;
>> + bio_set_dev(&bio, sb->s_bdev);
>> + bio_set_op_attrs(&bio, REQ_OP_READ, 0);
>> + bio_add_page(&bio, page, PAGE_SIZE, 0);
>> +
>> + ret = submit_bio_wait(&bio);
>> + if (ret)
>> + goto out;
>> +
>> + ret = -EINVAL;
>> + super = page_address(page);
>> + if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC)
>> + goto out;
>> + sbi->s_features = le64_to_cpu(super->s_features);
>> + if (zonefs_has_feature(sbi, ZONEFS_F_UID)) {
>> + sbi->s_uid = make_kuid(current_user_ns(),
>> + le32_to_cpu(super->s_uid));
>> + if (!uid_valid(sbi->s_uid))
>> + goto out;
>> + }
>> + if (zonefs_has_feature(sbi, ZONEFS_F_GID)) {
>> + sbi->s_gid = make_kgid(current_user_ns(),
>> + le32_to_cpu(super->s_gid));
>> + if (!gid_valid(sbi->s_gid))
>> + goto out;
>> + }
>> + if (zonefs_has_feature(sbi, ZONEFS_F_PERM))
>> + sbi->s_perm = le32_to_cpu(super->s_perm);
>
> Unknown feature bits are silently ignored. Is that intentional? Will
> all features be compat features? I would find it a little annoying to
> format (for example) a F_UID filesystem only to have some old kernel
> driver ignore it.
Good point. I will add checks for unknown feature bits.
>> +/*
>> + * On-disk super block (block 0).
>> + */
>> +struct zonefs_super {
>> +
>> + /* Magic number */
>> + __le32 s_magic;
>> +
>> + /* Features */
>> + __le64 s_features;
>> +
>> + /* 128-bit uuid */
>> + uuid_t s_uuid;
>> +
>> + /* UID/GID to use for files */
>> + __le32 s_uid;
>> + __le32 s_gid;
>> +
>> + /* File permissions */
>> + __le32 s_perm;
>> +
>> + /* Padding to 4K */
>> + __u8 s_reserved[4056];
>
> Hmm, I noticed that fill_super doesn't check that s_reserved is actually
> zero (or any specific value). You might consider enforcing that so that
> future you can add fields beyond s_perm without having to burn a
> s_features bit every time you do it.
Indeed. Will fix that.
> Also a little surprised there's no checksum field here to detect bit
> flips and such. ;)
Yep. I did have that on my to-do list but forgot to actually do it... Will fix
everything and send a v3.
Thank you for the comments. One more question: should I rebase on your
iomap-for-next branch or on iomap-5.4-merge branch ? Both branches look
identical right now.
Best regards.
--
Damien Le Moal
Western Digital Research
