Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614933 > unrolled thread

[PATCH 0/7] block: T10/DIF Fixes and cleanups v2

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] block: T10/DIF Fixes and cleanups v2 Dmitry Monakhov <dmonakhov@openvz.org> - 2017-04-03 09:30 +0200
    [PATCH 1/7] bio-integrity: Do not allocate integrity context for bio w/o data Dmitry Monakhov <dmonakhov@openvz.org> - 2017-04-03 09:30 +0200
      Re: [PATCH 1/7] bio-integrity: Do not allocate integrity context for  bio w/o data Christoph Hellwig <hch@infradead.org> - 2017-04-04 09:10 +0200
    [PATCH 6/7] T10: Move opencoded contants to common header Dmitry Monakhov <dmonakhov@openvz.org> - 2017-04-03 09:30 +0200
      Re: [PATCH 6/7] T10: Move opencoded contants to common header Christoph Hellwig <hch@infradead.org> - 2017-04-04 09:10 +0200
    Re: [PATCH 0/7] block: T10/DIF Fixes and cleanups v2 "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-04-03 23:20 +0200

#1614933 — [PATCH 0/7] block: T10/DIF Fixes and cleanups v2

This patch set fix various problems spotted during T10/DIF integrity machinery testing.

TOC:
## Fix various bugs in T10/DIF/DIX infrastructure
0001-bio-integrity-Do-not-allocate-integrity-context-for-fsync
0002-bio-integrity-save-original-iterator-for-verify-stage
0003-bio-integrity-bio_trim-should-truncate-integrity-vec
0004-bio-integrity-fix-interface-for-bio_integrity_trim
## Cleanup T10/DIF/DIX infrastructure
0005-bio-integrity-add-bio_integrity_setup-helper
0006-T10-Move-opencoded-contants-to-common-header
## General bulletproof protection for block layer
0007-Guard-bvec-iteration-logic-v2

Changes since V1
 - fix issues potted by kbuild bot
 - Replace BUG_ON with error logic for 7'th patch

Testcase: xfstest blockdev/003
https://github.com/dmonakhov/xfstests/commit/3c6509eaa83b9c17cd0bc95d73fcdd76e1c54a85

[toc] | [next] | [standalone]

#1614934 — [PATCH 1/7] bio-integrity: Do not allocate integrity context for bio w/o data

If bio has no data, such as ones from blkdev_issue_flush(),
then we have nothing to protect.

This patch prevent bugon like follows:

kfree_debugcheck: out of range ptr ac1fa1d106742a5ah
kernel BUG at mm/slab.c:2773!
invalid opcode: 0000 [#1] SMP
Modules linked in: bcache
CPU: 0 PID: 4428 Comm: xfs_io Tainted: G        W       4.11.0-rc4-ext4-00041-g2ef0043-dirty #43
Hardware name: Virtuozzo KVM, BIOS seabios-1.7.5-11.vz7.4 04/01/2014
task: ffff880137786440 task.stack: ffffc90000ba8000
RIP: 0010:kfree_debugcheck+0x25/0x2a
RSP: 0018:ffffc90000babde0 EFLAGS: 00010082
RAX: 0000000000000034 RBX: ac1fa1d106742a5a RCX: 0000000000000007
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88013f3ccb40
RBP: ffffc90000babde8 R08: 0000000000000000 R09: 0000000000000000
R10: 00000000fcb76420 R11: 00000000725172ed R12: 0000000000000282
R13: ffffffff8150e766 R14: ffff88013a145e00 R15: 0000000000000001
FS:  00007fb09384bf40(0000) GS:ffff88013f200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fd0172f9e40 CR3: 0000000137fa9000 CR4: 00000000000006f0
Call Trace:
 kfree+0xc8/0x1b3
 bio_integrity_free+0xc3/0x16b
 bio_free+0x25/0x66
 bio_put+0x14/0x26
 blkdev_issue_flush+0x7a/0x85
 blkdev_fsync+0x35/0x42
 vfs_fsync_range+0x8e/0x9f
 vfs_fsync+0x1c/0x1e
 do_fsync+0x31/0x4a
 SyS_fsync+0x10/0x14
 entry_SYSCALL_64_fastpath+0x1f/0xc2

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 block/bio-integrity.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 5384713..b5009a8 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -175,6 +175,9 @@ bool bio_integrity_enabled(struct bio *bio)
 	if (bio_op(bio) != REQ_OP_READ && bio_op(bio) != REQ_OP_WRITE)
 		return false;
 
+	if (!bio_sectors(bio))
+		return false;
+
 	/* Already protected? */
 	if (bio_integrity(bio))
 		return false;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1615738 — Re: [PATCH 1/7] bio-integrity: Do not allocate integrity context for bio w/o data

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1614935 — [PATCH 6/7] T10: Move opencoded contants to common header

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 block/t10-pi.c                   | 9 +++------
 drivers/scsi/lpfc/lpfc_scsi.c    | 5 +++--
 drivers/scsi/qla2xxx/qla_isr.c   | 8 ++++----
 drivers/target/target_core_sbc.c | 2 +-
 include/linux/t10-pi.h           | 3 +++
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/block/t10-pi.c b/block/t10-pi.c
index 2c97912..485cecd 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -28,9 +28,6 @@
 
 typedef __be16 (csum_fn) (void *, unsigned int);
 
-static const __be16 APP_ESCAPE = (__force __be16) 0xffff;
-static const __be32 REF_ESCAPE = (__force __be32) 0xffffffff;
-
 static __be16 t10_pi_crc_fn(void *data, unsigned int len)
 {
 	return cpu_to_be16(crc_t10dif(data, len));
@@ -82,7 +79,7 @@ static int t10_pi_verify(struct blk_integrity_iter *iter, csum_fn *fn,
 		switch (type) {
 		case 1:
 		case 2:
-			if (pi->app_tag == APP_ESCAPE)
+			if (pi->app_tag == T10_APP_ESCAPE)
 				goto next;
 
 			if (be32_to_cpu(pi->ref_tag) !=
@@ -95,8 +92,8 @@ static int t10_pi_verify(struct blk_integrity_iter *iter, csum_fn *fn,
 			}
 			break;
 		case 3:
-			if (pi->app_tag == APP_ESCAPE &&
-			    pi->ref_tag == REF_ESCAPE)
+			if (pi->app_tag == T10_APP_ESCAPE &&
+			    pi->ref_tag == T10_REF_ESCAPE)
 				goto next;
 			break;
 		}
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 54fd0c8..6f6b40e 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -26,6 +26,7 @@
 #include <linux/export.h>
 #include <linux/delay.h>
 #include <asm/unaligned.h>
+#include <linux/t10-pi.h>
 #include <linux/crc-t10dif.h>
 #include <net/checksum.h>
 
@@ -2934,8 +2935,8 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 				 * First check to see if a protection data
 				 * check is valid
 				 */
-				if ((src->ref_tag == 0xffffffff) ||
-				    (src->app_tag == 0xffff)) {
+				if ((src->ref_tag == T10_REF_ESCAPE) ||
+				    (src->app_tag == T10_APP_ESCAPE)) {
 					start_ref_tag++;
 					goto skipit;
 				}
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 3203367..ed4b302 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1950,9 +1950,9 @@ qla2x00_handle_dif_error(srb_t *sp, struct sts_entry_24xx *sts24)
 	 * For type     3: ref & app tag is all 'f's
 	 * For type 0,1,2: app tag is all 'f's
 	 */
-	if ((a_app_tag == 0xffff) &&
+	if ((a_app_tag == T10_APP_ESCAPE) &&
 	    ((scsi_get_prot_type(cmd) != SCSI_PROT_DIF_TYPE3) ||
-	     (a_ref_tag == 0xffffffff))) {
+	     (a_ref_tag == T10_REF_ESCAPE))) {
 		uint32_t blocks_done, resid;
 		sector_t lba_s = scsi_get_lba(cmd);
 
@@ -1994,9 +1994,9 @@ qla2x00_handle_dif_error(srb_t *sp, struct sts_entry_24xx *sts24)
 			spt = page_address(sg_page(sg)) + sg->offset;
 			spt += j;
 
-			spt->app_tag = 0xffff;
+			spt->app_tag = T10_APP_ESCAPE;
 			if (scsi_get_prot_type(cmd) == SCSI_PROT_DIF_TYPE3)
-				spt->ref_tag = 0xffffffff;
+				spt->ref_tag = T10_REF_ESCAPE;
 		}
 
 		return 0;
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index c194063..927ef44 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1446,7 +1446,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 				 (unsigned long long)sector, sdt->guard_tag,
 				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
 
-			if (sdt->app_tag == cpu_to_be16(0xffff)) {
+			if (sdt->app_tag == T10_APP_ESCAPE) {
 				dsg_off += block_size;
 				goto next;
 			}
diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index 9fba9dd..c96845c 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -24,6 +24,9 @@ enum t10_dif_type {
 	T10_PI_TYPE3_PROTECTION = 0x3,
 };
 
+static const __be16 T10_APP_ESCAPE = (__force __be16) 0xffff;
+static const __be32 T10_REF_ESCAPE = (__force __be32) 0xffffffff;
+
 /*
  * T10 Protection Information tuple.
  */
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1615735 — Re: [PATCH 6/7] T10: Move opencoded contants to common header

> -				if ((src->ref_tag == 0xffffffff) ||
> -				    (src->app_tag == 0xffff)) {
> +				if ((src->ref_tag == T10_REF_ESCAPE) ||
> +				    (src->app_tag == T10_APP_ESCAPE)) {

Please remove the inner braces while you're at it (also later in the
patch).

> index 9fba9dd..c96845c 100644
> --- a/include/linux/t10-pi.h
> +++ b/include/linux/t10-pi.h
> @@ -24,6 +24,9 @@ enum t10_dif_type {
>  	T10_PI_TYPE3_PROTECTION = 0x3,
>  };
>  
> +static const __be16 T10_APP_ESCAPE = (__force __be16) 0xffff;
> +static const __be32 T10_REF_ESCAPE = (__force __be32) 0xffffffff;

I'd do this as:

#define T10_APP_ESCAPE	cpu_to_be16(0xffff);
#define T10_REF_ESCAPE	cpu_to_be32(0xffffffff);

This avoids relying on the compiler to merge constants, and also gets
the endianess annotation right instead of force escaping it.

[toc] | [prev] | [next] | [standalone]

#1615558

Dmitry Monakhov <dmonakhov@openvz.org> writes:

Dmitry,

> This patch set fix various problems spotted during T10/DIF integrity
> machinery testing.
>
> TOC:
> ## Fix various bugs in T10/DIF/DIX infrastructure
> 0001-bio-integrity-Do-not-allocate-integrity-context-for-fsync
> 0002-bio-integrity-save-original-iterator-for-verify-stage
> 0003-bio-integrity-bio_trim-should-truncate-integrity-vec
> 0004-bio-integrity-fix-interface-for-bio_integrity_trim
> ## Cleanup T10/DIF/DIX infrastructure
> 0005-bio-integrity-add-bio_integrity_setup-helper
> 0006-T10-Move-opencoded-contants-to-common-header
> ## General bulletproof protection for block layer
> 0007-Guard-bvec-iteration-logic-v2

Looks like a nice cleanup of some of the things that have rotted a bit
as a result of the immutable bvec efforts. No major objections from
here. I'll try your series on my qual setup tomorrow to make sure
everything is working correctly.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web