[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Now, if writing pages and releasing compress blocks occur
simultaneously, and releasing cblocks is executed more than one time
to a file, then total block count of filesystem and block count of the
file could be incorrect and damaged.

We have to execute releasing compress blocks only one time for a file
without being interfered by writepages path.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/file.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4aab4b42d8ba..f7de2a1da528 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 	pgoff_t page_idx = 0, last_idx;
 	unsigned int released_blocks = 0;
 	int ret;
+	int writecount;
 
 	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
 		return -EOPNOTSUPP;
@@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 	if (ret)
 		return ret;
 
-	if (!F2FS_I(inode)->i_compr_blocks)
-		goto out;
-
 	f2fs_balance_fs(F2FS_I_SB(inode), true);
 
 	inode_lock(inode);
 
-	if (!IS_IMMUTABLE(inode)) {
-		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
-		f2fs_set_inode_flags(inode);
-		inode->i_ctime = current_time(inode);
-		f2fs_mark_inode_dirty_sync(inode, true);
+	writecount = atomic_read(&inode->i_writecount);
+	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
+		ret = -EBUSY;
+		goto out;
 	}
 
+	if (IS_IMMUTABLE(inode)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
+	if (ret)
+		goto out;
+
+	if (!F2FS_I(inode)->i_compr_blocks)
+		goto out;
+
+	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
+	f2fs_set_inode_flags(inode);
+	inode->i_ctime = current_time(inode);
+	f2fs_mark_inode_dirty_sync(inode, true);
+
 	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	down_write(&F2FS_I(inode)->i_mmap_sem);
 
@@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 
 	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	up_write(&F2FS_I(inode)->i_mmap_sem);
-
-	inode_unlock(inode);
 out:
+	inode_unlock(inode);
+
 	mnt_drop_write_file(filp);
 
 	if (ret >= 0) {
-- 
2.26.2.645.ge9eca65c58-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Chao Yu]

On 2020/5/8 17:29, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
> 
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Jaegeuk Kim]

Hi Daeho,

Please let me integrate this patch into the original patch since it is still in
the dev branch.

Thanks,

On 05/08, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
> 
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>  fs/f2fs/file.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4aab4b42d8ba..f7de2a1da528 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	pgoff_t page_idx = 0, last_idx;
>  	unsigned int released_blocks = 0;
>  	int ret;
> +	int writecount;
>  
>  	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
>  		return -EOPNOTSUPP;
> @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	if (ret)
>  		return ret;
>  
> -	if (!F2FS_I(inode)->i_compr_blocks)
> -		goto out;
> -
>  	f2fs_balance_fs(F2FS_I_SB(inode), true);
>  
>  	inode_lock(inode);
>  
> -	if (!IS_IMMUTABLE(inode)) {
> -		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> -		f2fs_set_inode_flags(inode);
> -		inode->i_ctime = current_time(inode);
> -		f2fs_mark_inode_dirty_sync(inode, true);
> +	writecount = atomic_read(&inode->i_writecount);
> +	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> +		ret = -EBUSY;
> +		goto out;
>  	}
>  
> +	if (IS_IMMUTABLE(inode)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> +	if (ret)
> +		goto out;
> +
> +	if (!F2FS_I(inode)->i_compr_blocks)
> +		goto out;
> +
> +	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> +	f2fs_set_inode_flags(inode);
> +	inode->i_ctime = current_time(inode);
> +	f2fs_mark_inode_dirty_sync(inode, true);
> +
>  	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>  	down_write(&F2FS_I(inode)->i_mmap_sem);
>  
> @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  
>  	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>  	up_write(&F2FS_I(inode)->i_mmap_sem);
> -
> -	inode_unlock(inode);
>  out:
> +	inode_unlock(inode);
> +
>  	mnt_drop_write_file(filp);
>  
>  	if (ret >= 0) {
> -- 
> 2.26.2.645.ge9eca65c58-goog


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Jaegeuk Kim]

On 05/08, Jaegeuk Kim wrote:
> Is this v2?

nvm. it seems the same version.

> 
> On 05/08, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> > 
> > Now, if writing pages and releasing compress blocks occur
> > simultaneously, and releasing cblocks is executed more than one time
> > to a file, then total block count of filesystem and block count of the
> > file could be incorrect and damaged.
> > 
> > We have to execute releasing compress blocks only one time for a file
> > without being interfered by writepages path.
> > 
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> > ---
> >  fs/f2fs/file.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 4aab4b42d8ba..f7de2a1da528 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >  	pgoff_t page_idx = 0, last_idx;
> >  	unsigned int released_blocks = 0;
> >  	int ret;
> > +	int writecount;
> >  
> >  	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> >  		return -EOPNOTSUPP;
> > @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (!F2FS_I(inode)->i_compr_blocks)
> > -		goto out;
> > -
> >  	f2fs_balance_fs(F2FS_I_SB(inode), true);
> >  
> >  	inode_lock(inode);
> >  
> > -	if (!IS_IMMUTABLE(inode)) {
> > -		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> > -		f2fs_set_inode_flags(inode);
> > -		inode->i_ctime = current_time(inode);
> > -		f2fs_mark_inode_dirty_sync(inode, true);
> > +	writecount = atomic_read(&inode->i_writecount);
> > +	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> > +		ret = -EBUSY;
> > +		goto out;
> >  	}
> >  
> > +	if (IS_IMMUTABLE(inode)) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (!F2FS_I(inode)->i_compr_blocks)
> > +		goto out;
> > +
> > +	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> > +	f2fs_set_inode_flags(inode);
> > +	inode->i_ctime = current_time(inode);
> > +	f2fs_mark_inode_dirty_sync(inode, true);
> > +
> >  	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >  	down_write(&F2FS_I(inode)->i_mmap_sem);
> >  
> > @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >  
> >  	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >  	up_write(&F2FS_I(inode)->i_mmap_sem);
> > -
> > -	inode_unlock(inode);
> >  out:
> > +	inode_unlock(inode);
> > +
> >  	mnt_drop_write_file(filp);
> >  
> >  	if (ret >= 0) {
> > -- 
> > 2.26.2.645.ge9eca65c58-goog
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Jaegeuk Kim]

Is this v2?

On 05/08, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
> 
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/file.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4aab4b42d8ba..f7de2a1da528 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	pgoff_t page_idx = 0, last_idx;
>  	unsigned int released_blocks = 0;
>  	int ret;
> +	int writecount;
>  
>  	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
>  		return -EOPNOTSUPP;
> @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	if (ret)
>  		return ret;
>  
> -	if (!F2FS_I(inode)->i_compr_blocks)
> -		goto out;
> -
>  	f2fs_balance_fs(F2FS_I_SB(inode), true);
>  
>  	inode_lock(inode);
>  
> -	if (!IS_IMMUTABLE(inode)) {
> -		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> -		f2fs_set_inode_flags(inode);
> -		inode->i_ctime = current_time(inode);
> -		f2fs_mark_inode_dirty_sync(inode, true);
> +	writecount = atomic_read(&inode->i_writecount);
> +	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> +		ret = -EBUSY;
> +		goto out;
>  	}
>  
> +	if (IS_IMMUTABLE(inode)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> +	if (ret)
> +		goto out;
> +
> +	if (!F2FS_I(inode)->i_compr_blocks)
> +		goto out;
> +
> +	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> +	f2fs_set_inode_flags(inode);
> +	inode->i_ctime = current_time(inode);
> +	f2fs_mark_inode_dirty_sync(inode, true);
> +
>  	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>  	down_write(&F2FS_I(inode)->i_mmap_sem);
>  
> @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  
>  	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>  	up_write(&F2FS_I(inode)->i_mmap_sem);
> -
> -	inode_unlock(inode);
>  out:
> +	inode_unlock(inode);
> +
>  	mnt_drop_write_file(filp);
>  
>  	if (ret >= 0) {
> -- 
> 2.26.2.645.ge9eca65c58-goog


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Now, if writing pages and releasing compress blocks occur
simultaneously, and releasing cblocks is executed more than one time
to a file, then total block count of filesystem and block count of the
file could be incorrect and damaged.

We have to execute releasing compress blocks only one time for a file
without being interfered by writepages path.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/file.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4aab4b42d8ba..f7de2a1da528 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 	pgoff_t page_idx = 0, last_idx;
 	unsigned int released_blocks = 0;
 	int ret;
+	int writecount;
 
 	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
 		return -EOPNOTSUPP;
@@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 	if (ret)
 		return ret;
 
-	if (!F2FS_I(inode)->i_compr_blocks)
-		goto out;
-
 	f2fs_balance_fs(F2FS_I_SB(inode), true);
 
 	inode_lock(inode);
 
-	if (!IS_IMMUTABLE(inode)) {
-		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
-		f2fs_set_inode_flags(inode);
-		inode->i_ctime = current_time(inode);
-		f2fs_mark_inode_dirty_sync(inode, true);
+	writecount = atomic_read(&inode->i_writecount);
+	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
+		ret = -EBUSY;
+		goto out;
 	}
 
+	if (IS_IMMUTABLE(inode)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
+	if (ret)
+		goto out;
+
+	if (!F2FS_I(inode)->i_compr_blocks)
+		goto out;
+
+	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
+	f2fs_set_inode_flags(inode);
+	inode->i_ctime = current_time(inode);
+	f2fs_mark_inode_dirty_sync(inode, true);
+
 	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	down_write(&F2FS_I(inode)->i_mmap_sem);
 
@@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 
 	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	up_write(&F2FS_I(inode)->i_mmap_sem);
-
-	inode_unlock(inode);
 out:
+	inode_unlock(inode);
+
 	mnt_drop_write_file(filp);
 
 	if (ret >= 0) {
-- 
2.26.2.645.ge9eca65c58-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Daeho Jeong]

Oops,

I will re-check it.

Thanks,

2020년 5월 8일 (금) 오후 4:09, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2020/5/8 14:58, Daeho Jeong wrote:
> > I moved checking i_compr_blocks phrase after calling inode_lock()
> > already, because we should check this after writing pages.
> >
> > So, if it fails to check i_compr_blocks, we need to call inode_unlock().
> >
> > Am I missing something?
>
> After applying this patch, I get this:
>
>         ret = mnt_want_write_file(filp);
>         if (ret)
>                 return ret;
>
>         if (!F2FS_I(inode)->i_compr_blocks)
>                 goto out;
>
>         f2fs_balance_fs(F2FS_I_SB(inode), true);
>
>         inode_lock(inode);
>
> >
> > 2020년 5월 8일 (금) 오후 3:50, Chao Yu <yuchao0@huawei.com>님이 작성:
> >>
> >> On 2020/5/8 12:25, Daeho Jeong wrote:
> >>> From: Daeho Jeong <daehojeong@google.com>
> >>>
> >>> Now, if writing pages and releasing compress blocks occur
> >>> simultaneously, and releasing cblocks is executed more than one time
> >>> to a file, then total block count of filesystem and block count of the
> >>> file could be incorrect and damaged.
> >>>
> >>> We have to execute releasing compress blocks only one time for a file
> >>> without being interfered by writepages path.
> >>>
> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>> ---
> >>>  fs/f2fs/file.c | 31 ++++++++++++++++++++++++-------
> >>>  1 file changed, 24 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index 4aab4b42d8ba..a92bc51b9b28 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >>>       pgoff_t page_idx = 0, last_idx;
> >>>       unsigned int released_blocks = 0;
> >>>       int ret;
> >>> +     int writecount;
> >>>
> >>>       if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> >>>               return -EOPNOTSUPP;
> >>
> >> Before inode_lock(), there is one case we may jump to out label, in
> >> this case, we may unlock inode incorrectly.
> >>
> >>         if (!F2FS_I(inode)->i_compr_blocks)
> >>                 goto out;
> >>
> >>> -
> >>> -     inode_unlock(inode);
> >>>  out:
> >>> +     inode_unlock(inode);
> >>> +
> >>>       mnt_drop_write_file(filp);
> >>
> >> Thanks,
> > .
> >


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Chao Yu]

On 2020/5/8 14:58, Daeho Jeong wrote:
> I moved checking i_compr_blocks phrase after calling inode_lock()
> already, because we should check this after writing pages.
> 
> So, if it fails to check i_compr_blocks, we need to call inode_unlock().
> 
> Am I missing something?

After applying this patch, I get this:

	ret = mnt_want_write_file(filp);
	if (ret)
		return ret;

	if (!F2FS_I(inode)->i_compr_blocks)
		goto out;

	f2fs_balance_fs(F2FS_I_SB(inode), true);

	inode_lock(inode);

> 
> 2020년 5월 8일 (금) 오후 3:50, Chao Yu <yuchao0@huawei.com>님이 작성:
>>
>> On 2020/5/8 12:25, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Now, if writing pages and releasing compress blocks occur
>>> simultaneously, and releasing cblocks is executed more than one time
>>> to a file, then total block count of filesystem and block count of the
>>> file could be incorrect and damaged.
>>>
>>> We have to execute releasing compress blocks only one time for a file
>>> without being interfered by writepages path.
>>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>  fs/f2fs/file.c | 31 ++++++++++++++++++++++++-------
>>>  1 file changed, 24 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 4aab4b42d8ba..a92bc51b9b28 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>>>       pgoff_t page_idx = 0, last_idx;
>>>       unsigned int released_blocks = 0;
>>>       int ret;
>>> +     int writecount;
>>>
>>>       if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
>>>               return -EOPNOTSUPP;
>>
>> Before inode_lock(), there is one case we may jump to out label, in
>> this case, we may unlock inode incorrectly.
>>
>>         if (!F2FS_I(inode)->i_compr_blocks)
>>                 goto out;
>>
>>> -
>>> -     inode_unlock(inode);
>>>  out:
>>> +     inode_unlock(inode);
>>> +
>>>       mnt_drop_write_file(filp);
>>
>> Thanks,
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Daeho Jeong]

I moved checking i_compr_blocks phrase after calling inode_lock()
already, because we should check this after writing pages.

So, if it fails to check i_compr_blocks, we need to call inode_unlock().

Am I missing something?

2020년 5월 8일 (금) 오후 3:50, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2020/5/8 12:25, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > Now, if writing pages and releasing compress blocks occur
> > simultaneously, and releasing cblocks is executed more than one time
> > to a file, then total block count of filesystem and block count of the
> > file could be incorrect and damaged.
> >
> > We have to execute releasing compress blocks only one time for a file
> > without being interfered by writepages path.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >  fs/f2fs/file.c | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 4aab4b42d8ba..a92bc51b9b28 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >       pgoff_t page_idx = 0, last_idx;
> >       unsigned int released_blocks = 0;
> >       int ret;
> > +     int writecount;
> >
> >       if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> >               return -EOPNOTSUPP;
>
> Before inode_lock(), there is one case we may jump to out label, in
> this case, we may unlock inode incorrectly.
>
>         if (!F2FS_I(inode)->i_compr_blocks)
>                 goto out;
>
> > -
> > -     inode_unlock(inode);
> >  out:
> > +     inode_unlock(inode);
> > +
> >       mnt_drop_write_file(filp);
>
> Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Chao Yu]

On 2020/5/8 12:25, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
> 
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>  fs/f2fs/file.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4aab4b42d8ba..a92bc51b9b28 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	pgoff_t page_idx = 0, last_idx;
>  	unsigned int released_blocks = 0;
>  	int ret;
> +	int writecount;
>  
>  	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
>  		return -EOPNOTSUPP;

Before inode_lock(), there is one case we may jump to out label, in
this case, we may unlock inode incorrectly.

	if (!F2FS_I(inode)->i_compr_blocks)
		goto out;

> -
> -	inode_unlock(inode);
>  out:
> +	inode_unlock(inode);
> +
>  	mnt_drop_write_file(filp);

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Now, if writing pages and releasing compress blocks occur
simultaneously, and releasing cblocks is executed more than one time
to a file, then total block count of filesystem and block count of the
file could be incorrect and damaged.

We have to execute releasing compress blocks only one time for a file
without being interfered by writepages path.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/file.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4aab4b42d8ba..a92bc51b9b28 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 	pgoff_t page_idx = 0, last_idx;
 	unsigned int released_blocks = 0;
 	int ret;
+	int writecount;
 
 	if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
 		return -EOPNOTSUPP;
@@ -3509,13 +3510,29 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 
 	inode_lock(inode);
 
-	if (!IS_IMMUTABLE(inode)) {
-		F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
-		f2fs_set_inode_flags(inode);
-		inode->i_ctime = current_time(inode);
-		f2fs_mark_inode_dirty_sync(inode, true);
+	writecount = atomic_read(&inode->i_writecount);
+	if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
+		ret = -EBUSY;
+		goto out;
 	}
 
+	if (IS_IMMUTABLE(inode)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
+	if (ret)
+		goto out;
+
+	if (!F2FS_I(inode)->i_compr_blocks)
+		goto out;
+
+	F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
+	f2fs_set_inode_flags(inode);
+	inode->i_ctime = current_time(inode);
+	f2fs_mark_inode_dirty_sync(inode, true);
+
 	down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	down_write(&F2FS_I(inode)->i_mmap_sem);
 
@@ -3554,9 +3571,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
 
 	up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 	up_write(&F2FS_I(inode)->i_mmap_sem);
-
-	inode_unlock(inode);
 out:
+	inode_unlock(inode);
+
 	mnt_drop_write_file(filp);
 
 	if (ret >= 0) {
-- 
2.26.2.526.g744177e7f7-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel