2024-01-11 06:44:31

by Chao Yu

Subject: [PATCH] f2fs: compress: fix to cover f2fs_disable_compressed_file() w/ i_sem

- f2fs_disable_compressed_file
- check inode_has_data
- f2fs_file_mmap
- mkwrite
- f2fs_get_block_locked
: update metadata in compressed
inode's disk layout
- fi->i_flags &= ~F2FS_COMPR_FL
- clear_inode_flag(inode, FI_COMPRESSED_FILE);

we should use i_sem lock to prevent above race case.

Meanwhile, this patch adds i_size check to restrict compress inode
conversion condition.

Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/f2fs.h | 18 ++++++++++++++++--
fs/f2fs/file.c | 5 ++---
2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 74729db0b381..e2e0ca45f881 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4406,19 +4406,33 @@ static inline int set_compress_context(struct inode *inode)
#endif
}

+static inline bool inode_has_data(struct inode *inode)
+{
+ return (S_ISREG(inode->i_mode) &&
+ (F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
+}
+
static inline bool f2fs_disable_compressed_file(struct inode *inode)
{
struct f2fs_inode_info *fi = F2FS_I(inode);

- if (!f2fs_compressed_file(inode))
+ f2fs_down_write(&F2FS_I(inode)->i_sem);
+
+ if (!f2fs_compressed_file(inode)) {
+ f2fs_up_write(&F2FS_I(inode)->i_sem);
return true;
- if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
+ }
+ if (f2fs_is_mmap_file(inode) || inode_has_data(inode)) {
+ f2fs_up_write(&F2FS_I(inode)->i_sem);
return false;
+ }

fi->i_flags &= ~F2FS_COMPR_FL;
stat_dec_compr_inode(inode);
clear_inode_flag(inode, FI_COMPRESSED_FILE);
f2fs_mark_inode_dirty_sync(inode, true);
+
+ f2fs_up_write(&F2FS_I(inode)->i_sem);
return true;
}

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 0e4c871d6aed..5e5df234eb92 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1926,8 +1926,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)

f2fs_down_write(&F2FS_I(inode)->i_sem);
if (!f2fs_may_compress(inode) ||
- (S_ISREG(inode->i_mode) &&
- F2FS_HAS_BLOCKS(inode))) {
+ inode_has_data(inode)) {
f2fs_up_write(&F2FS_I(inode)->i_sem);
return -EINVAL;
}
@@ -4011,7 +4010,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
goto out;
}

- if (F2FS_HAS_BLOCKS(inode)) {
+ if (inode_has_data(inode)) {
ret = -EFBIG;
goto out;
}
--
2.40.1

2024-01-16 18:29:52

by Jaegeuk Kim

Subject: Re: [PATCH] f2fs: compress: fix to cover f2fs_disable_compressed_file() w/ i_sem

On 01/11, Chao Yu wrote:
> - f2fs_disable_compressed_file
> - check inode_has_data
> - f2fs_file_mmap
> - mkwrite
> - f2fs_get_block_locked
> : update metadata in compressed
> inode's disk layout
> - fi->i_flags &= ~F2FS_COMPR_FL
> - clear_inode_flag(inode, FI_COMPRESSED_FILE);
>
> we should use i_sem lock to prevent above race case.
>
> Meanwhile, this patch adds i_size check to restrict compress inode
> conversion condition.

Sorry, what was the reason to check i_size?

>
> Fixes: 4c8ff7095bef ("f2fs: support data compression")
> Signed-off-by: Chao Yu <[email protected]>
> ---
> fs/f2fs/f2fs.h | 18 ++++++++++++++++--
> fs/f2fs/file.c | 5 ++---
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 74729db0b381..e2e0ca45f881 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4406,19 +4406,33 @@ static inline int set_compress_context(struct inode *inode)
> #endif
> }
>
> +static inline bool inode_has_data(struct inode *inode)
> +{
> + return (S_ISREG(inode->i_mode) &&
> + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
> +}
> +
> static inline bool f2fs_disable_compressed_file(struct inode *inode)
> {
> struct f2fs_inode_info *fi = F2FS_I(inode);
>
> - if (!f2fs_compressed_file(inode))
> + f2fs_down_write(&F2FS_I(inode)->i_sem);
> +
> + if (!f2fs_compressed_file(inode)) {
> + f2fs_up_write(&F2FS_I(inode)->i_sem);
> return true;
> - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
> + }
> + if (f2fs_is_mmap_file(inode) || inode_has_data(inode)) {
> + f2fs_up_write(&F2FS_I(inode)->i_sem);
> return false;
> + }
>
> fi->i_flags &= ~F2FS_COMPR_FL;
> stat_dec_compr_inode(inode);
> clear_inode_flag(inode, FI_COMPRESSED_FILE);
> f2fs_mark_inode_dirty_sync(inode, true);
> +
> + f2fs_up_write(&F2FS_I(inode)->i_sem);
> return true;
> }
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 0e4c871d6aed..5e5df234eb92 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1926,8 +1926,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>
> f2fs_down_write(&F2FS_I(inode)->i_sem);
> if (!f2fs_may_compress(inode) ||
> - (S_ISREG(inode->i_mode) &&
> - F2FS_HAS_BLOCKS(inode))) {
> + inode_has_data(inode)) {
> f2fs_up_write(&F2FS_I(inode)->i_sem);
> return -EINVAL;
> }
> @@ -4011,7 +4010,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> goto out;
> }
>
> - if (F2FS_HAS_BLOCKS(inode)) {
> + if (inode_has_data(inode)) {
> ret = -EFBIG;
> goto out;
> }
> --
> 2.40.1

2024-01-22 02:26:02

by Chao Yu

Subject: Re: [PATCH] f2fs: compress: fix to cover f2fs_disable_compressed_file() w/ i_sem

On 2024/1/17 2:29, Jaegeuk Kim wrote:
> On 01/11, Chao Yu wrote:
>> - f2fs_disable_compressed_file
>> - check inode_has_data
>> - f2fs_file_mmap
>> - mkwrite
>> - f2fs_get_block_locked
>> : update metadata in compressed
>> inode's disk layout
>> - fi->i_flags &= ~F2FS_COMPR_FL
>> - clear_inode_flag(inode, FI_COMPRESSED_FILE);
>>
>> we should use i_sem lock to prevent above race case.
>>
>> Meanwhile, this patch adds i_size check to restrict compress inode
>> conversion condition.
>
> Sorry, what was the reason to check i_size?

Oh, I don't see any race condition, let's remove it in v2.

Thanks,

>
>>
>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>> Signed-off-by: Chao Yu <[email protected]>
>> ---
>> fs/f2fs/f2fs.h | 18 ++++++++++++++++--
>> fs/f2fs/file.c | 5 ++---
>> 2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 74729db0b381..e2e0ca45f881 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4406,19 +4406,33 @@ static inline int set_compress_context(struct inode *inode)
>> #endif
>> }
>>
>> +static inline bool inode_has_data(struct inode *inode)
>> +{
>> + return (S_ISREG(inode->i_mode) &&
>> + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
>> +}
>> +
>> static inline bool f2fs_disable_compressed_file(struct inode *inode)
>> {
>> struct f2fs_inode_info *fi = F2FS_I(inode);
>>
>> - if (!f2fs_compressed_file(inode))
>> + f2fs_down_write(&F2FS_I(inode)->i_sem);
>> +
>> + if (!f2fs_compressed_file(inode)) {
>> + f2fs_up_write(&F2FS_I(inode)->i_sem);
>> return true;
>> - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
>> + }
>> + if (f2fs_is_mmap_file(inode) || inode_has_data(inode)) {
>> + f2fs_up_write(&F2FS_I(inode)->i_sem);
>> return false;
>> + }
>>
>> fi->i_flags &= ~F2FS_COMPR_FL;
>> stat_dec_compr_inode(inode);
>> clear_inode_flag(inode, FI_COMPRESSED_FILE);
>> f2fs_mark_inode_dirty_sync(inode, true);
>> +
>> + f2fs_up_write(&F2FS_I(inode)->i_sem);
>> return true;
>> }
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 0e4c871d6aed..5e5df234eb92 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1926,8 +1926,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>>
>> f2fs_down_write(&F2FS_I(inode)->i_sem);
>> if (!f2fs_may_compress(inode) ||
>> - (S_ISREG(inode->i_mode) &&
>> - F2FS_HAS_BLOCKS(inode))) {
>> + inode_has_data(inode)) {
>> f2fs_up_write(&F2FS_I(inode)->i_sem);
>> return -EINVAL;
>> }
>> @@ -4011,7 +4010,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
>> goto out;
>> }
>>
>> - if (F2FS_HAS_BLOCKS(inode)) {
>> + if (inode_has_data(inode)) {
>> ret = -EFBIG;
>> goto out;
>> }
>> --
>> 2.40.1