linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix race of pending_pages in decompression
@ 2020-12-04  0:58 Daeho Jeong
  2020-12-04  3:28 ` [f2fs-dev] " Eric Biggers
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  0:58 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

From: Daeho Jeong <daehojeong@google.com>

I found out f2fs_free_dic() is invoked in a wrong timing, but
f2fs_verify_bio() still needed the dic info and it triggered the
below kernel panic. It has been caused by the race condition of
pending_pages value between decompression and verity logic, when
the same compression cluster had been split in different bios.
By split bios, f2fs_verify_bio() ended up with decreasing
pending_pages value before it is reset to nr_cpages by
f2fs_decompress_pages() and caused the kernel panic.

[ 4416.564763] Unable to handle kernel NULL pointer dereference
               at virtual address 0000000000000000
...
[ 4416.896016] Workqueue: fsverity_read_queue f2fs_verity_work
[ 4416.908515] pc : fsverity_verify_page+0x20/0x78
[ 4416.913721] lr : f2fs_verify_bio+0x11c/0x29c
[ 4416.913722] sp : ffffffc019533cd0
[ 4416.913723] x29: ffffffc019533cd0 x28: 0000000000000402
[ 4416.913724] x27: 0000000000000001 x26: 0000000000000100
[ 4416.913726] x25: 0000000000000001 x24: 0000000000000004
[ 4416.913727] x23: 0000000000001000 x22: 0000000000000000
[ 4416.913728] x21: 0000000000000000 x20: ffffffff2076f9c0
[ 4416.913729] x19: ffffffff2076f9c0 x18: ffffff8a32380c30
[ 4416.913731] x17: ffffffc01f966d97 x16: 0000000000000298
[ 4416.913732] x15: 0000000000000000 x14: 0000000000000000
[ 4416.913733] x13: f074faec89ffffff x12: 0000000000000000
[ 4416.913734] x11: 0000000000001000 x10: 0000000000001000
[ 4416.929176] x9 : ffffffff20d1f5c7 x8 : 0000000000000000
[ 4416.929178] x7 : 626d7464ff286b6b x6 : ffffffc019533ade
[ 4416.929179] x5 : 000000008049000e x4 : ffffffff2793e9e0
[ 4416.929180] x3 : 000000008049000e x2 : ffffff89ecfa74d0
[ 4416.929181] x1 : 0000000000000c40 x0 : ffffffff2076f9c0
[ 4416.929184] Call trace:
[ 4416.929187]  fsverity_verify_page+0x20/0x78
[ 4416.929189]  f2fs_verify_bio+0x11c/0x29c
[ 4416.929192]  f2fs_verity_work+0x58/0x84
[ 4417.050667]  process_one_work+0x270/0x47c
[ 4417.055354]  worker_thread+0x27c/0x4d8
[ 4417.059784]  kthread+0x13c/0x320
[ 4417.063693]  ret_from_fork+0x10/0x18

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/compress.c |  4 ++--
 fs/f2fs/data.c     | 24 +++++++++++++++++++-----
 fs/f2fs/f2fs.h     |  1 +
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 87090da8693d..cdf72e153da0 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -803,8 +803,6 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity)
 	if (cops->destroy_decompress_ctx)
 		cops->destroy_decompress_ctx(dic);
 out_free_dic:
-	if (verity)
-		atomic_set(&dic->pending_pages, dic->nr_cpages);
 	if (!verity)
 		f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
 								ret, false);
@@ -1498,6 +1496,8 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
 	dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
 	dic->inode = cc->inode;
 	atomic_set(&dic->pending_pages, cc->nr_cpages);
+	if (fsverity_active(cc->inode))
+		atomic_set(&dic->verity_pages, cc->nr_cpages);
 	dic->cluster_idx = cc->cluster_idx;
 	dic->cluster_size = cc->cluster_size;
 	dic->log_cluster_size = cc->log_cluster_size;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 42254d3859c7..be0567dcace9 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -202,7 +202,7 @@ static void f2fs_verify_bio(struct bio *bio)
 		dic = (struct decompress_io_ctx *)page_private(page);
 
 		if (dic) {
-			if (atomic_dec_return(&dic->pending_pages))
+			if (atomic_dec_return(&dic->verity_pages))
 				continue;
 			f2fs_verify_pages(dic->rpages,
 						dic->cluster_size);
@@ -2266,15 +2266,29 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 					is_readahead ? REQ_RAHEAD : 0,
 					page->index, for_write);
 			if (IS_ERR(bio)) {
+				unsigned int remained = dic->nr_cpages - i;
+				bool release = false;
+
 				ret = PTR_ERR(bio);
 				dic->failed = true;
-				if (!atomic_sub_return(dic->nr_cpages - i,
-							&dic->pending_pages)) {
+
+				if (fsverity_active(inode)) {
+					if (!atomic_sub_return(remained,
+						&dic->verity_pages))
+						release = true;
+				} else {
+					if (!atomic_sub_return(remained,
+						&dic->pending_pages))
+						release = true;
+				}
+
+				if (release) {
 					f2fs_decompress_end_io(dic->rpages,
-							cc->cluster_size, true,
-							false);
+						cc->cluster_size, true,
+						false);
 					f2fs_free_dic(dic);
 				}
+
 				f2fs_put_dnode(&dn);
 				*bio_ret = NULL;
 				return ret;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 94d16bde5e24..f328f55fb0a0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1341,6 +1341,7 @@ struct decompress_io_ctx {
 	size_t rlen;			/* valid data length in rbuf */
 	size_t clen;			/* valid data length in cbuf */
 	atomic_t pending_pages;		/* in-flight compressed page count */
+	atomic_t verity_pages;		/* in-flight page count for verity */
 	bool failed;			/* indicate IO error during decompression */
 	void *private;			/* payload buffer for specified decompression algorithm */
 	void *private2;			/* extra payload buffer */
-- 
2.29.2.576.ga3fc446d84-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  0:58 [PATCH] f2fs: fix race of pending_pages in decompression Daeho Jeong
@ 2020-12-04  3:28 ` Eric Biggers
  2020-12-04  3:43   ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2020-12-04  3:28 UTC (permalink / raw)
  To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On Fri, Dec 04, 2020 at 09:58:47AM +0900, Daeho Jeong wrote:
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 87090da8693d..cdf72e153da0 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -803,8 +803,6 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity)
>  	if (cops->destroy_decompress_ctx)
>  		cops->destroy_decompress_ctx(dic);
>  out_free_dic:
> -	if (verity)
> -		atomic_set(&dic->pending_pages, dic->nr_cpages);
>  	if (!verity)
>  		f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
>  								ret, false);
> @@ -1498,6 +1496,8 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
>  	dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
>  	dic->inode = cc->inode;
>  	atomic_set(&dic->pending_pages, cc->nr_cpages);
> +	if (fsverity_active(cc->inode))
> +		atomic_set(&dic->verity_pages, cc->nr_cpages);
>  	dic->cluster_idx = cc->cluster_idx;
>  	dic->cluster_size = cc->cluster_size;
>  	dic->log_cluster_size = cc->log_cluster_size;

The check for fsverity_active() is wrong.  It looks like you need to know
whether the bio needs to go through the fs-verity data verification.  The
correct way to determine that is to check whether STEP_VERITY is enabled in the
bio's bio_post_read_ctx.  It's set by f2fs_grab_read_bio() when needed.

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  3:28 ` [f2fs-dev] " Eric Biggers
@ 2020-12-04  3:43   ` Daeho Jeong
  2020-12-04  4:01     ` Eric Biggers
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  3:43 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

STEP_VERITY is enabled by f2fs_need_verity() and the function is like below.
We already know the second condition (idx < DIV_ROUND_UP...) is
satisfied when invoking f2fs_alloc_dic().

static inline bool f2fs_need_verity(const struct inode *inode, pgoff_t idx)
{
        return fsverity_active(inode) &&
               idx < DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
}

2020년 12월 4일 (금) 오후 12:28, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Fri, Dec 04, 2020 at 09:58:47AM +0900, Daeho Jeong wrote:
> > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > index 87090da8693d..cdf72e153da0 100644
> > --- a/fs/f2fs/compress.c
> > +++ b/fs/f2fs/compress.c
> > @@ -803,8 +803,6 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity)
> >       if (cops->destroy_decompress_ctx)
> >               cops->destroy_decompress_ctx(dic);
> >  out_free_dic:
> > -     if (verity)
> > -             atomic_set(&dic->pending_pages, dic->nr_cpages);
> >       if (!verity)
> >               f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
> >                                                               ret, false);
> > @@ -1498,6 +1496,8 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
> >       dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
> >       dic->inode = cc->inode;
> >       atomic_set(&dic->pending_pages, cc->nr_cpages);
> > +     if (fsverity_active(cc->inode))
> > +             atomic_set(&dic->verity_pages, cc->nr_cpages);
> >       dic->cluster_idx = cc->cluster_idx;
> >       dic->cluster_size = cc->cluster_size;
> >       dic->log_cluster_size = cc->log_cluster_size;
>
> The check for fsverity_active() is wrong.  It looks like you need to know
> whether the bio needs to go through the fs-verity data verification.  The
> correct way to determine that is to check whether STEP_VERITY is enabled in the
> bio's bio_post_read_ctx.  It's set by f2fs_grab_read_bio() when needed.
>
> - Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  3:43   ` Daeho Jeong
@ 2020-12-04  4:01     ` Eric Biggers
  2020-12-04  4:31       ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2020-12-04  4:01 UTC (permalink / raw)
  To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On Fri, Dec 04, 2020 at 12:43:23PM +0900, Daeho Jeong wrote:
> STEP_VERITY is enabled by f2fs_need_verity() and the function is like below.
> We already know the second condition (idx < DIV_ROUND_UP...) is
> satisfied when invoking f2fs_alloc_dic().
> 
> static inline bool f2fs_need_verity(const struct inode *inode, pgoff_t idx)
> {
>         return fsverity_active(inode) &&
>                idx < DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
> }

Are you sure?  I thought that compression (and encryption) apply to the whole
file, including any Merkle tree blocks past i_size.

Also, even if you include the i_size check, it's still wrong to check
fsverity_active() in the middle of the I/O because FS_IOC_ENABLE_VERITY can
execute concurrently, causing fsverity_active() to return false at the beginning
of the I/O and true later in the I/O.  It needs to be checked only once, at the
beginning...

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  4:01     ` Eric Biggers
@ 2020-12-04  4:31       ` Daeho Jeong
  2020-12-04  4:48         ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  4:31 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

> Are you sure?  I thought that compression (and encryption) apply to the whole
> file, including any Merkle tree blocks past i_size.

This "dic" structure is only for "de"compression, so we don't need to
worry about going beyond i_size case.

> Also, even if you include the i_size check, it's still wrong to check
> fsverity_active() in the middle of the I/O because FS_IOC_ENABLE_VERITY can
> execute concurrently, causing fsverity_active() to return false at the beginning
> of the I/O and true later in the I/O.  It needs to be checked only once, at the
> beginning...

Got it. Our previous implementation didn't consider this case. Need to fix this.

Thanks,

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  4:31       ` Daeho Jeong
@ 2020-12-04  4:48         ` Daeho Jeong
  2020-12-04  4:51           ` Eric Biggers
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  4:48 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

Eric,

I have another question.
I understand enabling the verity can be possible in the middle of I/O.
Is the opposite way also possible? Actually, I couldn't find any
disabling function of it, though.

2020년 12월 4일 (금) 오후 1:31, Daeho Jeong <daeho43@gmail.com>님이 작성:
>
> > Are you sure?  I thought that compression (and encryption) apply to the whole
> > file, including any Merkle tree blocks past i_size.
>
> This "dic" structure is only for "de"compression, so we don't need to
> worry about going beyond i_size case.
>
> > Also, even if you include the i_size check, it's still wrong to check
> > fsverity_active() in the middle of the I/O because FS_IOC_ENABLE_VERITY can
> > execute concurrently, causing fsverity_active() to return false at the beginning
> > of the I/O and true later in the I/O.  It needs to be checked only once, at the
> > beginning...
>
> Got it. Our previous implementation didn't consider this case. Need to fix this.
>
> Thanks,

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  4:48         ` Daeho Jeong
@ 2020-12-04  4:51           ` Eric Biggers
  2020-12-04  5:00             ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2020-12-04  4:51 UTC (permalink / raw)
  To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On Fri, Dec 04, 2020 at 01:48:24PM +0900, Daeho Jeong wrote:
> Eric,
> 
> I have another question.
> I understand enabling the verity can be possible in the middle of I/O.
> Is the opposite way also possible? Actually, I couldn't find any
> disabling function of it, though.
> 

No, disabling fs-verity on a file isn't possible.

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  4:51           ` Eric Biggers
@ 2020-12-04  5:00             ` Daeho Jeong
  2020-12-04  5:18               ` Eric Biggers
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  5:00 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

I think I don't understand how verity works.
Right after verity is enabled on a file, is the verity logic working
for the whole file data area?
Or it's just working for the data area which is updated after verity is enabled?

2020년 12월 4일 (금) 오후 1:51, Eric Biggers <ebiggers3@gmail.com>님이 작성:
>
> On Fri, Dec 04, 2020 at 01:48:24PM +0900, Daeho Jeong wrote:
> > Eric,
> >
> > I have another question.
> > I understand enabling the verity can be possible in the middle of I/O.
> > Is the opposite way also possible? Actually, I couldn't find any
> > disabling function of it, though.
> >
>
> No, disabling fs-verity on a file isn't possible.
>
> - Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  5:00             ` Daeho Jeong
@ 2020-12-04  5:18               ` Eric Biggers
  2020-12-04  7:01                 ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2020-12-04  5:18 UTC (permalink / raw)
  To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On Fri, Dec 04, 2020 at 02:00:34PM +0900, Daeho Jeong wrote:
> I think I don't understand how verity works.
> Right after verity is enabled on a file, is the verity logic working
> for the whole file data area?
> Or it's just working for the data area which is updated after verity is enabled?
> 

It's for the whole file.

My point is just that if there is a bio that saw that verity isn't enabled yet
when it started and therefore STEP_VERITY didn't get set in the
bio_post_read_ctx (or the bio_post_read_ctx didn't get allocated due to one not
being needed), then the filesystem shouldn't change its mind and try to verify
the pages when the bio completes if verity happened to be enabled concurrently.
It's too late for that bio.

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  5:18               ` Eric Biggers
@ 2020-12-04  7:01                 ` Daeho Jeong
  2020-12-04 18:29                   ` Jaegeuk Kim
  0 siblings, 1 reply; 12+ messages in thread
From: Daeho Jeong @ 2020-12-04  7:01 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

Thanks for the explanation about verity.
I got your point. Thanks~

2020년 12월 4일 (금) 오후 2:18, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Fri, Dec 04, 2020 at 02:00:34PM +0900, Daeho Jeong wrote:
> > I think I don't understand how verity works.
> > Right after verity is enabled on a file, is the verity logic working
> > for the whole file data area?
> > Or it's just working for the data area which is updated after verity is enabled?
> >
>
> It's for the whole file.
>
> My point is just that if there is a bio that saw that verity isn't enabled yet
> when it started and therefore STEP_VERITY didn't get set in the
> bio_post_read_ctx (or the bio_post_read_ctx didn't get allocated due to one not
> being needed), then the filesystem shouldn't change its mind and try to verify
> the pages when the bio completes if verity happened to be enabled concurrently.
> It's too late for that bio.
>
> - Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04  7:01                 ` Daeho Jeong
@ 2020-12-04 18:29                   ` Jaegeuk Kim
  2020-12-05  3:40                     ` Daeho Jeong
  0 siblings, 1 reply; 12+ messages in thread
From: Jaegeuk Kim @ 2020-12-04 18:29 UTC (permalink / raw)
  To: Daeho Jeong
  Cc: Eric Biggers, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 12/04, Daeho Jeong wrote:
> Thanks for the explanation about verity.
> I got your point. Thanks~

Possible fix can be like this?

---
 fs/f2fs/compress.c |  2 --
 fs/f2fs/data.c     | 19 +++++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 89f73a7c8667..c5fee4d7ea72 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1491,8 +1491,6 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
 	dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
 	dic->inode = cc->inode;
 	atomic_set(&dic->pending_pages, cc->nr_cpages);
-	if (fsverity_active(cc->inode))
-		atomic_set(&dic->verity_pages, cc->nr_cpages);
 	dic->cluster_idx = cc->cluster_idx;
 	dic->cluster_size = cc->cluster_size;
 	dic->log_cluster_size = cc->log_cluster_size;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e3168f32f943..657fb562d7d4 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1035,7 +1035,8 @@ static inline bool f2fs_need_verity(const struct inode *inode, pgoff_t idx)
 
 static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 				      unsigned nr_pages, unsigned op_flag,
-				      pgoff_t first_idx, bool for_write)
+				      pgoff_t first_idx, bool for_write,
+				      bool for_verity)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct bio *bio;
@@ -1057,7 +1058,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 		post_read_steps |= 1 << STEP_DECRYPT;
 	if (f2fs_compressed_file(inode))
 		post_read_steps |= 1 << STEP_DECOMPRESS_NOWQ;
-	if (f2fs_need_verity(inode, first_idx))
+	if (for_verity && f2fs_need_verity(inode, first_idx))
 		post_read_steps |= 1 << STEP_VERITY;
 
 	if (post_read_steps) {
@@ -1087,7 +1088,7 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
 	struct bio *bio;
 
 	bio = f2fs_grab_read_bio(inode, blkaddr, 1, op_flags,
-					page->index, for_write);
+					page->index, for_write, true);
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
 
@@ -2141,7 +2142,7 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
 	if (bio == NULL) {
 		bio = f2fs_grab_read_bio(inode, block_nr, nr_pages,
 				is_readahead ? REQ_RAHEAD : 0, page->index,
-				false);
+				false, true);
 		if (IS_ERR(bio)) {
 			ret = PTR_ERR(bio);
 			bio = NULL;
@@ -2188,6 +2189,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 	const unsigned blkbits = inode->i_blkbits;
 	const unsigned blocksize = 1 << blkbits;
 	struct decompress_io_ctx *dic = NULL;
+	bool for_verity = false;
 	int i;
 	int ret = 0;
 
@@ -2253,6 +2255,11 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 		goto out_put_dnode;
 	}
 
+	if (fsverity_active(cc->inode)) {
+		atomic_set(&dic->verity_pages, cc->nr_cpages);
+		for_verity = true;
+	}
+
 	for (i = 0; i < dic->nr_cpages; i++) {
 		struct page *page = dic->cpages[i];
 		block_t blkaddr;
@@ -2272,7 +2279,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 		if (!bio) {
 			bio = f2fs_grab_read_bio(inode, blkaddr, nr_pages,
 					is_readahead ? REQ_RAHEAD : 0,
-					page->index, for_write);
+					page->index, for_write, for_verity);
 			if (IS_ERR(bio)) {
 				unsigned int remained = dic->nr_cpages - i;
 				bool release = false;
@@ -2280,7 +2287,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 				ret = PTR_ERR(bio);
 				dic->failed = true;
 
-				if (fsverity_active(inode)) {
+				if (for_verity) {
 					if (!atomic_sub_return(remained,
 						&dic->verity_pages))
 						release = true;
-- 
2.29.2.576.ga3fc446d84-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix race of pending_pages in decompression
  2020-12-04 18:29                   ` Jaegeuk Kim
@ 2020-12-05  3:40                     ` Daeho Jeong
  0 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2020-12-05  3:40 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Eric Biggers, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

Yep, we need to come back to v1 and enable verity in a unit of cluster.
Plus, as I told you, I'll prevent newly verity enalbed pages from
being merged with verity disabled bio.

Thanks,

2020년 12월 5일 (토) 오전 3:29, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
>
> On 12/04, Daeho Jeong wrote:
> > Thanks for the explanation about verity.
> > I got your point. Thanks~
>
> Possible fix can be like this?
>
> ---
>  fs/f2fs/compress.c |  2 --
>  fs/f2fs/data.c     | 19 +++++++++++++------
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 89f73a7c8667..c5fee4d7ea72 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1491,8 +1491,6 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
>         dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
>         dic->inode = cc->inode;
>         atomic_set(&dic->pending_pages, cc->nr_cpages);
> -       if (fsverity_active(cc->inode))
> -               atomic_set(&dic->verity_pages, cc->nr_cpages);
>         dic->cluster_idx = cc->cluster_idx;
>         dic->cluster_size = cc->cluster_size;
>         dic->log_cluster_size = cc->log_cluster_size;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e3168f32f943..657fb562d7d4 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1035,7 +1035,8 @@ static inline bool f2fs_need_verity(const struct inode *inode, pgoff_t idx)
>
>  static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
>                                       unsigned nr_pages, unsigned op_flag,
> -                                     pgoff_t first_idx, bool for_write)
> +                                     pgoff_t first_idx, bool for_write,
> +                                     bool for_verity)
>  {
>         struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>         struct bio *bio;
> @@ -1057,7 +1058,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
>                 post_read_steps |= 1 << STEP_DECRYPT;
>         if (f2fs_compressed_file(inode))
>                 post_read_steps |= 1 << STEP_DECOMPRESS_NOWQ;
> -       if (f2fs_need_verity(inode, first_idx))
> +       if (for_verity && f2fs_need_verity(inode, first_idx))
>                 post_read_steps |= 1 << STEP_VERITY;
>
>         if (post_read_steps) {
> @@ -1087,7 +1088,7 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
>         struct bio *bio;
>
>         bio = f2fs_grab_read_bio(inode, blkaddr, 1, op_flags,
> -                                       page->index, for_write);
> +                                       page->index, for_write, true);
>         if (IS_ERR(bio))
>                 return PTR_ERR(bio);
>
> @@ -2141,7 +2142,7 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>         if (bio == NULL) {
>                 bio = f2fs_grab_read_bio(inode, block_nr, nr_pages,
>                                 is_readahead ? REQ_RAHEAD : 0, page->index,
> -                               false);
> +                               false, true);
>                 if (IS_ERR(bio)) {
>                         ret = PTR_ERR(bio);
>                         bio = NULL;
> @@ -2188,6 +2189,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>         const unsigned blkbits = inode->i_blkbits;
>         const unsigned blocksize = 1 << blkbits;
>         struct decompress_io_ctx *dic = NULL;
> +       bool for_verity = false;
>         int i;
>         int ret = 0;
>
> @@ -2253,6 +2255,11 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>                 goto out_put_dnode;
>         }
>
> +       if (fsverity_active(cc->inode)) {
> +               atomic_set(&dic->verity_pages, cc->nr_cpages);
> +               for_verity = true;
> +       }
> +
>         for (i = 0; i < dic->nr_cpages; i++) {
>                 struct page *page = dic->cpages[i];
>                 block_t blkaddr;
> @@ -2272,7 +2279,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>                 if (!bio) {
>                         bio = f2fs_grab_read_bio(inode, blkaddr, nr_pages,
>                                         is_readahead ? REQ_RAHEAD : 0,
> -                                       page->index, for_write);
> +                                       page->index, for_write, for_verity);
>                         if (IS_ERR(bio)) {
>                                 unsigned int remained = dic->nr_cpages - i;
>                                 bool release = false;
> @@ -2280,7 +2287,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>                                 ret = PTR_ERR(bio);
>                                 dic->failed = true;
>
> -                               if (fsverity_active(inode)) {
> +                               if (for_verity) {
>                                         if (!atomic_sub_return(remained,
>                                                 &dic->verity_pages))
>                                                 release = true;
> --
> 2.29.2.576.ga3fc446d84-goog
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-12-05  3:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04  0:58 [PATCH] f2fs: fix race of pending_pages in decompression Daeho Jeong
2020-12-04  3:28 ` [f2fs-dev] " Eric Biggers
2020-12-04  3:43   ` Daeho Jeong
2020-12-04  4:01     ` Eric Biggers
2020-12-04  4:31       ` Daeho Jeong
2020-12-04  4:48         ` Daeho Jeong
2020-12-04  4:51           ` Eric Biggers
2020-12-04  5:00             ` Daeho Jeong
2020-12-04  5:18               ` Eric Biggers
2020-12-04  7:01                 ` Daeho Jeong
2020-12-04 18:29                   ` Jaegeuk Kim
2020-12-05  3:40                     ` Daeho Jeong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).