linux-kernel - Re: [PATCH v4 2/2] erofs: add 'fsoffset' mount option for file-backed & bdev-based mounts

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <93df9b24-3a37-48ff-aa21-5f27a76ecf90@gmail.com>
Date: Thu, 8 May 2025 20:53:14 +0800
From: Sheng Yong <shengyong2021@...il.com>
To: Gao Xiang <hsiangkao@...ux.alibaba.com>, xiang@...nel.org,
 chao@...nel.org, zbestahu@...il.com, jefflexu@...ux.alibaba.com,
 dhavale@...gle.com, kzak@...hat.com
Cc: linux-erofs@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
 wangshuai12@...omi.com, Sheng Yong <shengyong1@...omi.com>
Subject: Re: [PATCH v4 2/2] erofs: add 'fsoffset' mount option for file-backed
 & bdev-based mounts

On 5/8/25 12:09, Gao Xiang wrote:
> Hi Yong,
> 
[...]
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index 4ac188d5d894..10656bd986bd 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -43,6 +43,7 @@ struct erofs_device_info {
>>       char *path;
>>       struct erofs_fscache *fscache;
>>       struct file *file;
>> +    loff_t off;
>>       struct dax_device *dax_dev;
>>       u64 dax_part_off;
> 
> I wonder if it's possible to combine off as dax_part_off since
> they are the same functionality...

Hi, Xiang,

Thanks for the review. Yes, they almost have the same functionality.
Will combine them.
> 
>> @@ -199,6 +200,7 @@ enum {
>>   struct erofs_buf {
>>       struct address_space *mapping;
>>       struct file *file;
>> +    loff_t off;
> 
> I wonder if there is some other way to check
> if it's a metabuf, so that we could just use sbi->dif0.off..
> 
> But not sure.

It seems no specific fields could tell directly whether a buf is a
metabuf. But I'll have a try.
> 
>>       struct page *page;
>>       void *base;
>>   };
> 
> ..
> 
>> +        if (sb->s_bdev)
>> +            devsz = bdev_nr_bytes(sb->s_bdev);
>> +        else if (erofs_is_fileio_mode(sbi))
>> +            devsz = i_size_read(file_inode(sbi->dif0.file));
>> +        else
>> +            return invalfc(fc, "fsoffset only supports file or bdev 
>> backing");
>> +        if (sbi->dif0.off + (1 << sbi->blkszbits) > devsz)
>> +            return invalfc(fc, "fsoffset exceeds device size");
> 
> I wonder if those checks are really necessary? even it exceeds
> the device size, it won't find the valid on-disk superblock then.

Will remove this check.
> 
>> +    }
>> +
[...]
>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>> index 0671184d9cf1..671527b63c6d 100644
>> --- a/fs/erofs/zdata.c
>> +++ b/fs/erofs/zdata.c
>> @@ -1624,7 +1624,8 @@ static void z_erofs_submit_queue(struct 
>> z_erofs_frontend *f,
>>                    bool *force_fg, bool readahead)
>>   {
>>       struct super_block *sb = f->inode->i_sb;
>> -    struct address_space *mc = MNGD_MAPPING(EROFS_SB(sb));
>> +    struct erofs_sb_info *sbi = EROFS_SB(sb);
>> +    struct address_space *mc = MNGD_MAPPING(sbi);
>>       struct z_erofs_pcluster **qtail[NR_JOBQUEUES];
>>       struct z_erofs_decompressqueue *q[NR_JOBQUEUES];
>>       struct z_erofs_pcluster *pcl, *next;
>> @@ -1673,12 +1674,15 @@ static void z_erofs_submit_queue(struct 
>> z_erofs_frontend *f,
>>               if (bio && (cur != last_pa ||
>>                       bio->bi_bdev != mdev.m_bdev)) {
>>   drain_io:
>> -                if (erofs_is_fileio_mode(EROFS_SB(sb)))
>> +                if (erofs_is_fileio_mode(sbi)) {
>>                       erofs_fileio_submit_bio(bio);
>> -                else if (erofs_is_fscache_mode(sb))
>> +                } else if (erofs_is_fscache_mode(sb)) {
>>                       erofs_fscache_submit_bio(bio);
>> -                else
>> +                } else {
>> +                    bio->bi_iter.bi_sector +=
>> +                        sbi->dif0.off >> SECTOR_SHIFT;
> 
> How about multi-device? I guess we should modify
> erofs_map_dev() directly rather than callers.
Will fix fsoffset on multi device in the next version.

thanks,
shengyong
> 
> Thanks,
> Gao Xiang