[RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Milan Broz]

In dm-integrity target we register integrity profile that have
both generate_fn and verify_fn callbacks set to NULL.

This is used if dm-integrity is stacked under a dm-crypt device
for authenticated encryption (integrity payload contains authentication
tag and IV seed).

In this case the verification is done through own crypto API
processing inside dm-crypt; integrity profile is only holder
of these data. (And memory is owned by dm-crypt as well.)

After the commit (and previous changes)
  Commit 7c20f11680a441df09de7235206f70115fbf6290
  Author: Christoph Hellwig <hch@lst.de>
  Date:   Mon Jul 3 16:58:43 2017 -0600

    bio-integrity: stop abusing bi_end_io

we get this crash:

: BUG: unable to handle kernel NULL pointer dereference at   (null)
: IP:   (null)
: *pde = 00000000
...
:
: Workqueue: kintegrityd bio_integrity_verify_fn
: task: f48ae180 task.stack: f4b5c000
: EIP:   (null)
: EFLAGS: 00210286 CPU: 0
: EAX: f4b5debc EBX: 00001000 ECX: 00000001 EDX: 00000000
: ESI: 00001000 EDI: ed25f000 EBP: f4b5dee8 ESP: f4b5dea4
:  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
: CR0: 80050033 CR2: 00000000 CR3: 32823000 CR4: 001406d0
: Call Trace:
:  ? bio_integrity_process+0xe3/0x1e0
:  bio_integrity_verify_fn+0xea/0x150
:  process_one_work+0x1c7/0x5c0
:  worker_thread+0x39/0x380
:  kthread+0xd6/0x110
:  ? process_one_work+0x5c0/0x5c0
:  ? kthread_worker_fn+0x100/0x100
:  ? kthread_worker_fn+0x100/0x100
:  ret_from_fork+0x19/0x24
: Code:  Bad EIP value.
: EIP:   (null) SS:ESP: 0068:f4b5dea4
: CR2: 0000000000000000

Patch just skip the whole verify workqueue if verify_fn is set to NULL.

Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
 block/bio-integrity.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 83e92beb3c9f..b9d1580bfc13 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -387,7 +387,9 @@ static void bio_integrity_verify_fn(struct work_struct *work)
  */
 bool __bio_integrity_endio(struct bio *bio)
 {
-	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status) {
+	struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
+
+	if (bi->profile->verify_fn && bio_op(bio) == REQ_OP_READ && !bio->bi_status) {
 		struct bio_integrity_payload *bip = bio_integrity(bio);
 
 		INIT_WORK(&bip->bip_work, bio_integrity_verify_fn);
-- 
2.13.3

Re: [RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Christoph Hellwig]

On Wed, Aug 02, 2017 at 02:27:50PM +0200, Milan Broz wrote:
> In dm-integrity target we register integrity profile that have
> both generate_fn and verify_fn callbacks set to NULL.
> 
> This is used if dm-integrity is stacked under a dm-crypt device
> for authenticated encryption (integrity payload contains authentication
> tag and IV seed).
> 
> In this case the verification is done through own crypto API
> processing inside dm-crypt; integrity profile is only holder
> of these data. (And memory is owned by dm-crypt as well.)

Maybe that's where the problem lies?  You're abusing the integrity
payload for something that is not end to end data integrity at all
and then wonder why it breaks?  Also the commit that introduced your
code had absolutely no review by Martin or any of the core block
folks.

The right fix is to revert the dm-crypt commit.

Re: [RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Milan Broz]

On 08/02/2017 02:55 PM, Christoph Hellwig wrote:
> On Wed, Aug 02, 2017 at 02:27:50PM +0200, Milan Broz wrote:
>> In dm-integrity target we register integrity profile that have
>> both generate_fn and verify_fn callbacks set to NULL.
>>
>> This is used if dm-integrity is stacked under a dm-crypt device
>> for authenticated encryption (integrity payload contains authentication
>> tag and IV seed).
>>
>> In this case the verification is done through own crypto API
>> processing inside dm-crypt; integrity profile is only holder
>> of these data. (And memory is owned by dm-crypt as well.)
> 
> Maybe that's where the problem lies?  You're abusing the integrity
> payload for something that is not end to end data integrity at all
> and then wonder why it breaks?  Also the commit that introduced your
> code had absolutely no review by Martin or any of the core block
> folks.
> 
> The right fix is to revert the dm-crypt commit.

Hi Christoph,

Why revert? The data there are integrity protection data, just it must to be
processed together with encryption through crypto API call (it is just AEAD,
it must take data + auth tag together).

And the integrity profile is perfect interface for this, we register
own profile through the proper interface.
(Any other solution for per-sector metadata would be worse, I tried...)

And yes, I agree that there should have been review form Martin,
TBH it was not intention to squeeze it there, seems everyone forgot
about this (but the patch went through dm-devel for months).
(Initially it was part of my research work so I care about idea and not patches
just it becomes production code too fast :-)

Anyway, we have it in 4.12 and from my point of view (as cryptsetup
maintainer) this is very needed functionality.

So what do you suggest to do now? I do not think we break anything,
just we need to agree how to use that integrity extension for additional
profiles.

Milan

Re: [RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Martin K. Petersen]

Milan,

> And the integrity profile is perfect interface for this, we register
> own profile through the proper interface.  (Any other solution for
> per-sector metadata would be worse, I tried...)

The DM use case seems a bit weird and I would like to take a closer look
later (a storm took out my power and internet so I'm a bit logistically
impaired right now). But the intent of the integrity infrastructure was
to be able to carry arbitrary metadata pinned to an I/O. The callback
hooks in the profile were really only intended for the case where the
block layer itself needed to generate and verify.

We already do sanity checking on the callback pointers in the prep
function. I guess don't have a problem doing the same in the completion
path.

Fundamentally, though, the verify function should only ever be called if
the profile has BLK_INTEGRITY_VERIFY set.

Previously that was done in the prep function by only adding the verify
endio hook when it was needed. Christoph's patch to kill the double
endio changed that subtly (FWIW, Jens originally objected to having an
integrity conditional in the regular bio completion path. That's why the
verification handling abused the endio function).

Anyway. So I think that the BLK_INTEGRITY_VERIFY logic needs to be
carried over to __bio_integrity_endio()...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Milan Broz]

On 08/02/2017 04:11 PM, Martin K. Petersen wrote:
>> And the integrity profile is perfect interface for this, we register
>> own profile through the proper interface.  (Any other solution for
>> per-sector metadata would be worse, I tried...)
> 
> The DM use case seems a bit weird and I would like to take a closer look
> later (a storm took out my power and internet so I'm a bit logistically
> impaired right now).

Hi Martin,

thanks!

I think it is actually pretty straightforward (if it can be said
about anything in the device-mapper little walled garden :-)

For the authenticated encryption (on the sector level) we cannot use
a length-preserving encryption (as it is dm-crypt in normal mode) but we need
some per-sector metadata to store additional authentication tag.

This is exactly what DIF/T10-PI already does, just we need more flexibility
(and larger metadata to use cryptographically sound modes).

So we developed dm-integrity target that takes a normal block device,
stores per-sector metadata on-disk in special sectors (interleaved with normal
data sectors).
(There is a journalling to provide atomicity of data + metadata writes.)

The dm-integrity can calculate some non-cryptographic integrity data itself,
but this mode does not use block integrity extension at all so it is not important here.

The second use case (that is currently broken by the block layer 4.13 changes)
is that dm-integrity can register a new integrity profile for the virtual device
(on top of normal block device) and present data sectors as normal bio layer and
metadata as the bio-integrity layer (dm-crypt will receive bio with the metadata
in integrity fields).

IOW dm-integrity is just "emulator" for the integrity-enabled block device,
just with configurable per-sector metadata size.

Then we can place dm-crypt above this device and process the bio the same way
as dm-crypt does, just it will add authentication tag mapped to the metadata
for that special integrity profile.

The dm-crypt already creates bio clones (and already owns the bio clone memory)
so we do exactly the same for integrity part of bio (clearing the BIP_BLOCK_INTEGRITY
flag to indicate that block layer should not free this memory).

Otherwise I think we use bio the exact way how the driver should register
and alloc memory for own integrity profile (see dm_crypt_integrity_io_alloc function).

Milan


> But the intent of the integrity infrastructure was
> to be able to carry arbitrary metadata pinned to an I/O. The callback
> hooks in the profile were really only intended for the case where the
> block layer itself needed to generate and verify.
> 
> We already do sanity checking on the callback pointers in the prep
> function. I guess don't have a problem doing the same in the completion
> path.
> 
> Fundamentally, though, the verify function should only ever be called if
> the profile has BLK_INTEGRITY_VERIFY set.
> 
> Previously that was done in the prep function by only adding the verify
> endio hook when it was needed. Christoph's patch to kill the double
> endio changed that subtly (FWIW, Jens originally objected to having an
> integrity conditional in the regular bio completion path. That's why the
> verification handling abused the endio function).
> 
> Anyway. So I think that the BLK_INTEGRITY_VERIFY logic needs to be
> carried over to __bio_integrity_endio()...
> 

[PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Mikulas Patocka]

On Wed, 2 Aug 2017, Christoph Hellwig wrote:

> On Wed, Aug 02, 2017 at 02:27:50PM +0200, Milan Broz wrote:
> > In dm-integrity target we register integrity profile that have
> > both generate_fn and verify_fn callbacks set to NULL.
> > 
> > This is used if dm-integrity is stacked under a dm-crypt device
> > for authenticated encryption (integrity payload contains authentication
> > tag and IV seed).
> > 
> > In this case the verification is done through own crypto API
> > processing inside dm-crypt; integrity profile is only holder
> > of these data. (And memory is owned by dm-crypt as well.)
> 
> Maybe that's where the problem lies?  You're abusing the integrity
> payload for something that is not end to end data integrity at all
> and then wonder why it breaks?  Also the commit that introduced your
> code had absolutely no review by Martin or any of the core block
> folks.
> 
> The right fix is to revert the dm-crypt commit.

That dm-crypt commit that uses bio integrity payload came 3 months before 
7c20f11680a441df09de7235206f70115fbf6290 and it was already present in 
4.12.

The patch 7c20f11680a441df09de7235206f70115fbf6290 not only breaks 
dm-crypt and dm-integrity. It also breaks regular integrity payload usage 
when stacking drivers are used.

Suppose that a bio goes the through the device mapper stack. At each level 
a clone bio is allocated and forwarded to a lower level. It eventually 
reaches the request-based physical disk driver.

In the kernel 4.12, when the bio reaches the request-based driver, the 
function blk_queue_bio is called, it calls bio_integrity_prep, 
bio_integrity_prep saves the bio->bi_end_io in the structure 
bio_integrity_payload and sets bio->bi_end_io = bio_integrity_endio. When 
the bio is finished, bio_integrity_endio is called, it performs the 
verification (a call to bio_integrity_verify_fn), restores bio->bi_end_io 
and calls it.

The patch 7c20f11680a441df09de7235206f70115fbf6290 changes it so that 
bio->bi_end_io is not changed and bio_integrity_endio is called directly 
from bio_endio if the bio has integrity payload. The unintended 
consequence of this change is that when a request with integrity payload 
is finished and bio_endio is called for each cloned bio, the verification 
routine bio_integrity_verify_fn is called for every level in the stack (it 
used to be called only for the lowest level in 4.12). At different levels 
in the stack, the bio may have different bi_sector value, so the repeated 
verification can't succeed.

I think the patch 7c20f11680a441df09de7235206f70115fbf6290 should be 
reverted and it should be reworked for the next merge window, so that it 
calls bio_integrity_verify_fn only for the lowest level.

Mikulas



From: Mikulas Patocka <mpatocka@redhat.co>

The patch 7c20f11680a441df09de7235206f70115fbf6290 ("bio-integrity: stop 
abusing bi_end_io") changes the code so that it doesn't replace 
bio_end_io with bio_integrity_endio and calls bio_integrity_endio directly 
from bio_endio.

The unintended consequence of this change is that when a bio with 
integrity payload is passed through the device stack, bio_integrity_endio 
is called for each level of the stack (it used to be called only for the 
lowest level before).

This breaks dm-integrity and dm-crypt and it also causes verification 
errors when a bio with regular integrity payload is passed through the 
device mapper stack.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index f733beab6ca2..8df4eb103ba9 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -102,7 +102,7 @@ EXPORT_SYMBOL(bio_integrity_alloc);
  * Description: Used to free the integrity portion of a bio. Usually
  * called from bio_free().
  */
-static void bio_integrity_free(struct bio *bio)
+void bio_integrity_free(struct bio *bio)
 {
 	struct bio_integrity_payload *bip = bio_integrity(bio);
 	struct bio_set *bs = bio->bi_pool;
@@ -120,8 +120,8 @@ static void bio_integrity_free(struct bio *bio)
 	}
 
 	bio->bi_integrity = NULL;
-	bio->bi_opf &= ~REQ_INTEGRITY;
 }
+EXPORT_SYMBOL(bio_integrity_free);
 
 /**
  * bio_integrity_add_page - Attach integrity metadata
@@ -326,6 +326,12 @@ bool bio_integrity_prep(struct bio *bio)
 		offset = 0;
 	}
 
+	/* Install custom I/O completion handler if read verify is enabled */
+	if (bio_data_dir(bio) == READ) {
+		bip->bip_end_io = bio->bi_end_io;
+		bio->bi_end_io = bio_integrity_endio;
+	}
+
 	/* Auto-generate integrity metadata if this is a write */
 	if (bio_data_dir(bio) == WRITE) {
 		bio_integrity_process(bio, &bio->bi_iter,
@@ -369,12 +375,13 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 		bio->bi_status = BLK_STS_IOERR;
 	}
 
-	bio_integrity_free(bio);
+	/* Restore original bio completion handler */
+	bio->bi_end_io = bip->bip_end_io;
 	bio_endio(bio);
 }
 
 /**
- * __bio_integrity_endio - Integrity I/O completion function
+ * bio_integrity_endio - Integrity I/O completion function
  * @bio:	Protected bio
  * @error:	Pointer to errno
  *
@@ -385,19 +392,27 @@ static void bio_integrity_verify_fn(struct work_struct *work)
  * in process context.	This function postpones completion
  * accordingly.
  */
-bool __bio_integrity_endio(struct bio *bio)
+void bio_integrity_endio(struct bio *bio)
 {
-	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status) {
-		struct bio_integrity_payload *bip = bio_integrity(bio);
+	struct bio_integrity_payload *bip = bio_integrity(bio);
 
-		INIT_WORK(&bip->bip_work, bio_integrity_verify_fn);
-		queue_work(kintegrityd_wq, &bip->bip_work);
-		return false;
+	BUG_ON(bip->bip_bio != bio);
+
+	/* In case of an I/O error there is no point in verifying the
+	 * integrity metadata.  Restore original bio end_io handler
+	 * and run it.
+	 */
+	if (bio->bi_status) {
+		bio->bi_end_io = bip->bip_end_io;
+		bio_endio(bio);
+
+		return;
 	}
 
-	bio_integrity_free(bio);
-	return true;
+	INIT_WORK(&bip->bip_work, bio_integrity_verify_fn);
+	queue_work(kintegrityd_wq, &bip->bip_work);
 }
+EXPORT_SYMBOL(bio_integrity_endio);
 
 /**
  * bio_integrity_advance - Advance integrity vector
diff --git a/block/bio.c b/block/bio.c
index 9cabf5d0be20..a6b225324a61 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -243,6 +243,9 @@ struct bio_vec *bvec_alloc(gfp_t gfp_mask, int nr, unsigned long *idx,
 void bio_uninit(struct bio *bio)
 {
 	bio_disassociate_task(bio);
+
+	if (bio_integrity(bio))
+		bio_integrity_free(bio);
 }
 EXPORT_SYMBOL(bio_uninit);
 
@@ -1810,8 +1813,6 @@ void bio_endio(struct bio *bio)
 again:
 	if (!bio_remaining_done(bio))
 		return;
-	if (!bio_integrity_endio(bio))
-		return;
 
 	/*
 	 * Need to have a real endio function for chained bios, otherwise
diff --git a/block/blk.h b/block/blk.h
index 3a3d715bd725..01ebb8185f6b 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -81,21 +81,10 @@ static inline void blk_queue_enter_live(struct request_queue *q)
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 void blk_flush_integrity(void);
-bool __bio_integrity_endio(struct bio *);
-static inline bool bio_integrity_endio(struct bio *bio)
-{
-	if (bio_integrity(bio))
-		return __bio_integrity_endio(bio);
-	return true;
-}
 #else
 static inline void blk_flush_integrity(void)
 {
 }
-static inline bool bio_integrity_endio(struct bio *bio)
-{
-	return true;
-}
 #endif
 
 void blk_timeout_work(struct work_struct *work);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7b1cf4ba0902..1eba19580185 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -320,6 +320,8 @@ struct bio_integrity_payload {
 
 	struct bvec_iter	bip_iter;
 
+	bio_end_io_t		*bip_end_io;	/* saved I/O completion fn */
+
 	unsigned short		bip_slab;	/* slab the bip came from */
 	unsigned short		bip_vcnt;	/* # of integrity bio_vecs */
 	unsigned short		bip_max_vcnt;	/* integrity bio_vec slots */
@@ -737,8 +739,10 @@ struct biovec_slab {
 		bip_for_each_vec(_bvl, _bio->bi_integrity, _iter)
 
 extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
+extern void bio_integrity_free(struct bio *);
 extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
 extern bool bio_integrity_prep(struct bio *);
+extern void bio_integrity_endio(struct bio *);
 extern void bio_integrity_advance(struct bio *, unsigned int);
 extern void bio_integrity_trim(struct bio *);
 extern int bio_integrity_clone(struct bio *, struct bio *, gfp_t);
@@ -768,6 +772,11 @@ static inline bool bio_integrity_prep(struct bio *bio)
 	return true;
 }
 
+static inline void bio_integrity_free(struct bio *bio)
+{
+	return;
+}
+
 static inline int bio_integrity_clone(struct bio *bio, struct bio *bio_src,
 				      gfp_t gfp_mask)
 {

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Christoph Hellwig]

On Thu, Aug 03, 2017 at 10:10:55AM -0400, Mikulas Patocka wrote:
> That dm-crypt commit that uses bio integrity payload came 3 months before 
> 7c20f11680a441df09de7235206f70115fbf6290 and it was already present in 
> 4.12.

And on it's own that isn't an argument if your usage is indeed wrong,
and that's why we are having this discussion.

> The patch 7c20f11680a441df09de7235206f70115fbf6290 changes it so that 
> bio->bi_end_io is not changed and bio_integrity_endio is called directly 
> from bio_endio if the bio has integrity payload. The unintended 
> consequence of this change is that when a request with integrity payload 
> is finished and bio_endio is called for each cloned bio, the verification 
> routine bio_integrity_verify_fn is called for every level in the stack (it 
> used to be called only for the lowest level in 4.12). At different levels 
> in the stack, the bio may have different bi_sector value, so the repeated 
> verification can't succeed.

We can simply add another bio flag to get back to the previous
behavior.  That being said thing to do in the end is to verify it
at the top of the stack, and not the bottom eventuall.  I can cook
up a patch for that.

> I think the patch 7c20f11680a441df09de7235206f70115fbf6290 should be 
> reverted and it should be reworked for the next merge window, so that it 
> calls bio_integrity_verify_fn only for the lowest level.

And with this you will just reintroduce the memory leak for
on-stack bios we've fixed.

I'll take a look at not calling the verify function for every level,
which is wrong, and in the meantime we can discuss the uses and abuses
of the bio integrity code by dm in the separate subthread.

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Mikulas Patocka]

On Sat, 5 Aug 2017, Christoph Hellwig wrote:

> 
> On Thu, Aug 03, 2017 at 10:10:55AM -0400, Mikulas Patocka wrote:
> > That dm-crypt commit that uses bio integrity payload came 3 months before 
> > 7c20f11680a441df09de7235206f70115fbf6290 and it was already present in 
> > 4.12.
> 
> And on it's own that isn't an argument if your usage is indeed wrong,
> and that's why we are having this discussion.
> 
> > The patch 7c20f11680a441df09de7235206f70115fbf6290 changes it so that 
> > bio->bi_end_io is not changed and bio_integrity_endio is called directly 
> > from bio_endio if the bio has integrity payload. The unintended 
> > consequence of this change is that when a request with integrity payload 
> > is finished and bio_endio is called for each cloned bio, the verification 
> > routine bio_integrity_verify_fn is called for every level in the stack (it 
> > used to be called only for the lowest level in 4.12). At different levels 
> > in the stack, the bio may have different bi_sector value, so the repeated 
> > verification can't succeed.
> 
> We can simply add another bio flag to get back to the previous
> behavior.  That being said thing to do in the end is to verify it
> at the top of the stack, and not the bottom eventuall.  I can cook
> up a patch for that.

The sector number in the integrity tag must match the physical sector 
number. So, it must be verified at the bottom.

If the sector number in the integrity tag matched the logical sector 
number in the logical volume, it would create a lot of problems - for 
example, it would be impossible to move the logical volume and it would be 
impossible to copy the full physical volume.

> > I think the patch 7c20f11680a441df09de7235206f70115fbf6290 should be 
> > reverted and it should be reworked for the next merge window, so that it 
> > calls bio_integrity_verify_fn only for the lowest level.
> 
> And with this you will just reintroduce the memory leak for
> on-stack bios we've fixed.
> 
> I'll take a look at not calling the verify function for every level,
> which is wrong, and in the meantime we can discuss the uses and abuses
> of the bio integrity code by dm in the separate subthread.

It would be usefull if dm-crypt can put the autenticated tag directly into 
the integrity data. But for that usage, the lowest level physical driver 
must not generate or verify the integrity data - it must just read them 
and write them.

Mikulas

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Martin K. Petersen]

Christoph,

> We can simply add another bio flag to get back to the previous
> behavior.  That being said thing to do in the end is to verify it
> at the top of the stack, and not the bottom eventuall.  I can cook
> up a patch for that.

Yeah, the original code was careful about only adding the verification
hook to the top bio.

A bio flag is probably the path of least resistance. It already exists,
actually: BIP_BLOCK_INTEGRITY. But we'll need to make sure it gets
masked out when a bio is cloned. And then we can key off of that and
REQ_OP_READ in the endio function.

I prefer that approach to reverting Christoph's commit.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Martin K. Petersen]

Mikulas,

> The sector number in the integrity tag must match the physical sector 
> number. So, it must be verified at the bottom.

The ref tag seed matches the submitter block number (typically block
layer sector for the top device) and is remapped to and from the LBA by
the SCSI disk driver or HBA firmware.

So the verification is designed to be done by the top level entity that
attaches the protection information to the bio. In this case
bio_integrity_prep().

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC PATCH] bio-integrity: Fix regression if profile verify_fn is NULL

[Thorsten Leemhuis]

TWIMC: This issue is tracked in the regression reports for Linux 4.13
(http://bit.ly/lnxregrep413 ) with this id:

Linux-Regression-ID: lr#35498d

Please include this line in the comment section of patches that are
supposed to fix the issue. Please also mention the string once in other
mailinglist threads or different bug tracking entries if you or someone
else start to discuss the issue there. By including that string you make
it a whole lot easier to track where an issue gets discussed and how far
patches to fix it have made it. For more details on this please see
here: http://bit.ly/lnxregtrackid

Thx for your help. And thx to Milan for pointing me to this regression.
 Ciao, Thorsten

On 02.08.2017 14:27, Milan Broz wrote:
> In dm-integrity target we register integrity profile that have
> both generate_fn and verify_fn callbacks set to NULL.
> 
> This is used if dm-integrity is stacked under a dm-crypt device
> for authenticated encryption (integrity payload contains authentication
> tag and IV seed).
> 
> In this case the verification is done through own crypto API
> processing inside dm-crypt; integrity profile is only holder
> of these data. (And memory is owned by dm-crypt as well.)
> 
> After the commit (and previous changes)
>   Commit 7c20f11680a441df09de7235206f70115fbf6290
>   Author: Christoph Hellwig <hch@lst.de>
>   Date:   Mon Jul 3 16:58:43 2017 -0600
> 
>     bio-integrity: stop abusing bi_end_io
> 
> we get this crash:
> 
> : BUG: unable to handle kernel NULL pointer dereference at   (null)
> : IP:   (null)
> : *pde = 00000000
> ...
> :
> : Workqueue: kintegrityd bio_integrity_verify_fn
> : task: f48ae180 task.stack: f4b5c000
> : EIP:   (null)
> : EFLAGS: 00210286 CPU: 0
> : EAX: f4b5debc EBX: 00001000 ECX: 00000001 EDX: 00000000
> : ESI: 00001000 EDI: ed25f000 EBP: f4b5dee8 ESP: f4b5dea4
> :  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> : CR0: 80050033 CR2: 00000000 CR3: 32823000 CR4: 001406d0
> : Call Trace:
> :  ? bio_integrity_process+0xe3/0x1e0
> :  bio_integrity_verify_fn+0xea/0x150
> :  process_one_work+0x1c7/0x5c0
> :  worker_thread+0x39/0x380
> :  kthread+0xd6/0x110
> :  ? process_one_work+0x5c0/0x5c0
> :  ? kthread_worker_fn+0x100/0x100
> :  ? kthread_worker_fn+0x100/0x100
> :  ret_from_fork+0x19/0x24
> : Code:  Bad EIP value.
> : EIP:   (null) SS:ESP: 0068:f4b5dea4
> : CR2: 0000000000000000
> 
> Patch just skip the whole verify workqueue if verify_fn is set to NULL.
> 
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> ---
>  block/bio-integrity.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 83e92beb3c9f..b9d1580bfc13 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -387,7 +387,9 @@ static void bio_integrity_verify_fn(struct work_struct *work)
>   */
>  bool __bio_integrity_endio(struct bio *bio)
>  {
> -	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status) {
> +	struct blk_integrity *bi = bdev_get_integrity(bio->bi_bdev);
> +
> +	if (bi->profile->verify_fn && bio_op(bio) == REQ_OP_READ && !bio->bi_status) {
>  		struct bio_integrity_payload *bip = bio_integrity(bio);
>  
>  		INIT_WORK(&bip->bip_work, bio_integrity_verify_fn);
> 

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Mikulas Patocka]

On Sat, 5 Aug 2017, Martin K. Petersen wrote:

> Mikulas,
> 
> > The sector number in the integrity tag must match the physical sector 
> > number. So, it must be verified at the bottom.
> 
> The ref tag seed matches the submitter block number (typically block
> layer sector for the top device) and is remapped to and from the LBA by
> the SCSI disk driver or HBA firmware.
> 
> So the verification is designed to be done by the top level entity that
> attaches the protection information to the bio. In this case
> bio_integrity_prep().

bio_integrity_prep is called by blk_queue_bio - that is the lowest level 
physical disk, not the top level. It is not called by device mapper.

Documentation/block/data-integrity.txt says that bio_integrity_prep() can 
be called by integrity-aware filesystem, but it seems that no such 
filesystem exists.

If you create the integrity tag at or above device mapper level, you will 
run into problems because the same device can be accessed using device 
mapper and using physical volume /dev/sd*. If you create integrity tags at 
device mapper level, they will contain device mapper's logical sector 
number and the sector number won't match if you access the device directly 
using /dev/sd*.

> -- 
> Martin K. Petersen	Oracle Linux Engineering

Mikulas

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Martin K. Petersen]

Mikulas,

> If you create the integrity tag at or above device mapper level, you
> will run into problems because the same device can be accessed using
> device mapper and using physical volume /dev/sd*. If you create
> integrity tags at device mapper level, they will contain device
> mapper's logical sector number and the sector number won't match if
> you access the device directly using /dev/sd*.

For writes, the bip seed value is adjusted every time a bio is cloned,
sliced and diced as it traverses partitioning/MD/DM. And then at the
bottom of the stack, the ref tag values in the protection information
buffer are verified against the adjusted seed value and remapped
according to the starting logical block number. The reverse is taking
place for reads.

This is much faster than doing a remapping of the actual protection
buffer values every time the I/O transitions from one address space to
the other. In addition, some HBA hardware allows us to program the PI
engine with the seed value. So the submitter value to LBA conversion can
be done on the fly in hardware.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Milan Broz]

Hi,

On 08/07/2017 05:48 PM, Martin K. Petersen wrote:
> 
>> If you create the integrity tag at or above device mapper level, you
>> will run into problems because the same device can be accessed using
>> device mapper and using physical volume /dev/sd*. If you create
>> integrity tags at device mapper level, they will contain device
>> mapper's logical sector number and the sector number won't match if
>> you access the device directly using /dev/sd*.
> 
> For writes, the bip seed value is adjusted every time a bio is cloned,
> sliced and diced as it traverses partitioning/MD/DM. And then at the
> bottom of the stack, the ref tag values in the protection information
> buffer are verified against the adjusted seed value and remapped
> according to the starting logical block number. The reverse is taking
> place for reads.
> 
> This is much faster than doing a remapping of the actual protection
> buffer values every time the I/O transitions from one address space to
> the other. In addition, some HBA hardware allows us to program the PI
> engine with the seed value. So the submitter value to LBA conversion can
> be done on the fly in hardware.

Yes, this is how enterprise storage works and how the PI was designed.

For authenticated encryption with additional data (AEAD) we have to authenticate
the sector number as well (as part or additional data - authenticated
but not encrypted part).

Unfortunately the whole design cannot work this way if the AEAD is implemented
on some higher layer. (Higher layer has a big advantage that you do not need to trust
underlying storage stack because you will detect even intentional tampering with data.)
 So, only the layer that own the keys (here dmcrypt) can calculate or verify auth. tags
(and then decrypt data). Nobody else can "remap" this sector in tag - without the key
you cannot recalculate auth. tags.

In other words, we just treat the whole additional data (delivered in bio-integrity)
as "Application tag" (if using DIF terminology...)


Anyway, could we please fix the 4.13 kernel now to not crash with that
dm-integrity use? This is urgent for people that already play with dm-integrity from 4.12.


If you want a following discussion, I would really welcome to see what exactly
is wrong because I think we used the bio-integrity interface through existing
API and according to doc.
Just we are unfortunately the first user for own profile (different than
these defined in T10).
And I think it can help in future to people that will try to implement
bio-integrity-aware filesystem as well.

Thanks,
Milan

Re: [PATCH] bio-integrity: revert "stop abusing bi_end_io"

[Christoph Hellwig]

On Sat, Aug 05, 2017 at 04:19:30PM -0400, Martin K. Petersen wrote:
> A bio flag is probably the path of least resistance. It already exists,
> actually: BIP_BLOCK_INTEGRITY. But we'll need to make sure it gets
> masked out when a bio is cloned. And then we can key off of that and
> REQ_OP_READ in the endio function.

bio_integrity_clone doesn't even clone it - it allocates a new
bip and then only copies over bip_vec, bip_vcnt and bit_iter.

I'll preparate a patch for this and will send it out together with
the orginal hotfix for the verify_fn, which also needs a trivial
line length fix anyway.