fix fs-integrity for non-PI metadata v2

fix fs-integrity for non-PI metadata v2

[Christoph Hellwig]

Hi all,

this series fixes incorrect calls into the PI generation helpers
when using fs-integrity on devics with non-PI metadata.

Changes since v1:
 - fix two more null pointer dereferences

(I though I already sent this out yesterday, but can't find it in the
archives.  At least this way I could pick up two more reviews from the
post as part of the larger lazy bounce buffer series.  Sorry if you
somehow got this twice)

Diffstat:
 block/bio-integrity-auto.c    |   10 ++--------
 block/bio-integrity-fs.c      |    6 +++++-
 fs/iomap/bio.c                |    2 +-
 include/linux/bio-integrity.h |    3 +++
 4 files changed, 11 insertions(+), 10 deletions(-)

[PATCH 1/4] block: remove bip_should_check

[Christoph Hellwig]

There is no benefit in using this helper over the simple flags check.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>
---
 block/bio-integrity-auto.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/bio-integrity-auto.c b/block/bio-integrity-auto.c
index b1c733ecfd2e..43ac9f338183 100644
--- a/block/bio-integrity-auto.c
+++ b/block/bio-integrity-auto.c
@@ -45,10 +45,6 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 }
 
 #define BIP_CHECK_FLAGS (BIP_CHECK_GUARD | BIP_CHECK_REFTAG | BIP_CHECK_APPTAG)
-static bool bip_should_check(struct bio_integrity_payload *bip)
-{
-	return bip->bip_flags & BIP_CHECK_FLAGS;
-}
 
 /**
  * __bio_integrity_endio - Integrity I/O completion function
@@ -66,7 +62,7 @@ bool __bio_integrity_endio(struct bio *bio)
 		container_of(bip, struct bio_integrity_data, bip);
 
 	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status &&
-	    bip_should_check(bip)) {
+	    (bip->bip_flags & BIP_CHECK_FLAGS)) {
 		INIT_WORK(&bid->work, bio_integrity_verify_fn);
 		queue_work(kintegrityd_wq, &bid->work);
 		return false;
@@ -99,7 +95,7 @@ void bio_integrity_prep(struct bio *bio, unsigned int action)
 		bio_integrity_setup_default(bio);
 
 	/* Auto-generate integrity metadata if this is a write */
-	if (bio_data_dir(bio) == WRITE && bip_should_check(&bid->bip))
+	if (bio_data_dir(bio) == WRITE && (bid->bip.bip_flags & BIP_CHECK_FLAGS))
 		bio_integrity_generate(bio);
 	else
 		bid->saved_bio_iter = bio->bi_iter;
-- 
2.53.0

Re: [PATCH 1/4] block: remove bip_should_check

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[PATCH 2/4] block: lift BIP_CHECK_FLAGS to include/linux/bio-integrity.h

[Christoph Hellwig]

To allow for users outside of bio-integrity-auto.c.  Also add a little
comment explaining it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>
---
 block/bio-integrity-auto.c    | 2 --
 include/linux/bio-integrity.h | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/bio-integrity-auto.c b/block/bio-integrity-auto.c
index 43ac9f338183..9456dcffd17a 100644
--- a/block/bio-integrity-auto.c
+++ b/block/bio-integrity-auto.c
@@ -44,8 +44,6 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 	bio_endio(bio);
 }
 
-#define BIP_CHECK_FLAGS (BIP_CHECK_GUARD | BIP_CHECK_REFTAG | BIP_CHECK_APPTAG)
-
 /**
  * __bio_integrity_endio - Integrity I/O completion function
  * @bio:	Protected bio
diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
index c3dda32fd803..0ea2a8bf7efb 100644
--- a/include/linux/bio-integrity.h
+++ b/include/linux/bio-integrity.h
@@ -17,6 +17,9 @@ enum bip_flags {
 	BIP_MEMPOOL		= 1 << 15, /* buffer backed by mempool */
 };
 
+/* flags that require generate/verify action. */
+#define BIP_CHECK_FLAGS (BIP_CHECK_GUARD | BIP_CHECK_REFTAG | BIP_CHECK_APPTAG)
+
 struct bio_integrity_payload {
 	struct bvec_iter	bip_iter;
 
-- 
2.53.0

Re: [PATCH 2/4] block: lift BIP_CHECK_FLAGS to include/linux/bio-integrity.h

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[PATCH 3/4] block: handle nogenerate/noverify properly in fs-integrity

[Christoph Hellwig]

Check the BIP_CHECK flags before generating or verifying PI information,
otherwise this can be incorrectly called for non-PI metadata and
cause generation of incorrect metadata and crashed in the verification
handler.

The new behavior matches that of the block layer auto-generated
metadata.

Fixes: 0bde8a12b554 ("block: add fs_bio_integrity helpers")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>
---
 block/bio-integrity-fs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/bio-integrity-fs.c b/block/bio-integrity-fs.c
index 9c5fe5fa8f0d..692403dfa047 100644
--- a/block/bio-integrity-fs.c
+++ b/block/bio-integrity-fs.c
@@ -46,7 +46,8 @@ void fs_bio_integrity_free(struct bio *bio)
 
 void fs_bio_integrity_generate(struct bio *bio)
 {
-	if (fs_bio_integrity_alloc(bio))
+	if (fs_bio_integrity_alloc(bio) &&
+	    (bio_integrity(bio)->bip_flags & BIP_CHECK_FLAGS))
 		bio_integrity_generate(bio);
 }
 EXPORT_SYMBOL_GPL(fs_bio_integrity_generate);
@@ -60,6 +61,9 @@ int fs_bio_integrity_verify(struct bio *bio, sector_t sector, unsigned int size)
 		.bi_size	= size,
 	};
 
+	if (!bip || !(bip->bip_flags & BIP_CHECK_FLAGS))
+		return 0;
+
 	/*
 	 * Reinitialize bip->bip_iter.
 	 *
-- 
2.53.0

Re: [PATCH 3/4] block: handle nogenerate/noverify properly in fs-integrity

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[PATCH 4/4] iomap: don't free integrity payload that doesn't exist

[Christoph Hellwig]

fs_bio_integrity_alloc might not allocate a bio integrity payload if PI
verification is disabled on the block device.  Check for that case before
calling fs_bio_integirty_free in iomap_bio_read_folio_range_sync to
avoid a NULL pointer dereferences.

Make the branch cover the PI verification as well - while
fs_bio_integrity_verify works without an integrity payload, it requires
one to actually do useful work.

Fixes: 0b10a370529c ("iomap: support T10 protection information")
Cc: <stable@vger.kernel.org> # v7.1
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/bio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/iomap/bio.c b/fs/iomap/bio.c
index dc8ac7e370a5..30ef78a66b4f 100644
--- a/fs/iomap/bio.c
+++ b/fs/iomap/bio.c
@@ -179,7 +179,7 @@ int iomap_bio_read_folio_range_sync(const struct iomap_iter *iter,
 	if (srcmap->flags & IOMAP_F_INTEGRITY)
 		fs_bio_integrity_alloc(&bio);
 	error = submit_bio_wait(&bio);
-	if (srcmap->flags & IOMAP_F_INTEGRITY) {
+	if (bio_integrity(&bio)) {
 		if (!error)
 			error = fs_bio_integrity_verify(&bio, sector, len);
 		fs_bio_integrity_free(&bio);
-- 
2.53.0

Re: [PATCH 4/4] iomap: don't free integrity payload that doesn't exist

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 4/4] iomap: don't free integrity payload that doesn't exist

[Anuj gupta]

Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>

Re: [PATCH 4/4] iomap: don't free integrity payload that doesn't exist

[Martin K. Petersen]

Christoph,

> fs_bio_integrity_alloc might not allocate a bio integrity payload if PI
> verification is disabled on the block device.  Check for that case before
> calling fs_bio_integirty_free in iomap_bio_read_folio_range_sync to
                 ^^^^^^^^^
Nit: Typo.

-- 
Martin K. Petersen

Re: fix fs-integrity for non-PI metadata v2

[Martin K. Petersen]

Christoph,

> this series fixes incorrect calls into the PI generation helpers when
> using fs-integrity on devics with non-PI metadata.

Looks OK.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen