[PATCH 0/3] scsi_debug: fix data integrity support

[PATCH 0/3] scsi_debug: fix data integrity support

[Akinobu Mita]

When I tried testing the data integrity support in scsi_debug on x86_32,
I got CONFIG_DEBUG_HIGHMEM warnings and protection errors.  This was
triggered due to misused kmap_atomic/kunmap_atomic.

And then, while I was testing the fix of the above issue with several
combination with module parameters dix and dif, I found that doing
'modprobe scsi_debug dif=0 dix=1' causes kernel crash.

This patch set includes these fixes and cleanup which is related to data
integrity support.

Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org

Akinobu Mita (3):
  scsi_debug: fix data integrity support on highmem machine
  scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1
  scsi_debug: simplify offset calculation for dif_storep

 drivers/scsi/scsi_debug.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] scsi_debug: fix data integrity support on highmem machine

[Akinobu Mita]

kmap_atomic() is now using stack based implementation and doesn't take
the KM_type argument anymore.  So nesting kmap_atomic() calls must be
properly stacked.

This fixes nesting kmap_atomic() calls for scsi_sglist and scsi_prot_sglist
in prot_verify_write().

This also fixes another issue that invalid kmap address is used for
kunmap_atomic(): the kmap address 'daddr' is incremented in the loop for
each data page, and it can reach the next page boundary.

These problems trigger CONFIG_DEBUG_HIGHMEM warnings, protection errors,
and kernel crash when doing I/O for the storage installed by
'modprobe scsi_debug dif=1 dix=1' on x86_32 with highmem.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index aea4c2e..8fd30a5 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1814,12 +1814,12 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
-	paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
 	ppage_offset = 0;
 
 	/* For each data page */
 	scsi_for_each_sg(SCpnt, dsgl, scsi_sg_count(SCpnt), i) {
 		daddr = kmap_atomic(sg_page(dsgl)) + dsgl->offset;
+		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
 
 		/* For each sector-sized chunk in data page */
 		for (j = 0 ; j < dsgl->length ; j += scsi_debug_sector_size) {
@@ -1904,19 +1904,18 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
 
-		kunmap_atomic(daddr);
+		kunmap_atomic(paddr);
+		kunmap_atomic(daddr - dsgl->length);
 	}
 
-	kunmap_atomic(paddr);
-
 	dix_writes++;
 
 	return 0;
 
 out:
 	dif_errors++;
-	kunmap_atomic(daddr);
 	kunmap_atomic(paddr);
+	kunmap_atomic(daddr);
 	return ret;
 }
 
-- 
1.8.1.4

[PATCH 2/3] scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1

[Akinobu Mita]

The protection info dif_storep is allocated only when parameter dif is
not zero.  But it will be accessed when reading or writing to the storage
installed with parameter dix is not zero.

So kernel crashes if scsi_debug module is loaded with parameters dix=1 and
dif=0.

This fixes it by making dif_storep available if parameter dix is not zero
instead of checking if parameter dif is not zero.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8fd30a5..ffbdb4c 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3306,7 +3306,7 @@ static int __init scsi_debug_init(void)
 	if (scsi_debug_num_parts > 0)
 		sdebug_build_parts(fake_storep, sz);
 
-	if (scsi_debug_dif) {
+	if (scsi_debug_dix) {
 		int dif_size;
 
 		dif_size = sdebug_store_sectors * sizeof(struct sd_dif_tuple);
-- 
1.8.1.4

[PATCH 3/3] scsi_debug: simplify offset calculation for dif_storep

[Akinobu Mita]

dif_storep is declared as pointer to unsigned char type.  But it is
actually used to store vmalloced array of struct sd_dif_tuple.

This changes the type of dif_storep to the pointer to struct sd_dif_tuple.
It simplifies offset calculation for dif_storep and enables to remove
hardcoded size of struct sd_dif_tuple.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ffbdb4c..e69cbc2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -258,7 +258,7 @@ struct sdebug_queued_cmd {
 static struct sdebug_queued_cmd queued_arr[SCSI_DEBUG_CANQUEUE];
 
 static unsigned char * fake_storep;	/* ramdisk storage */
-static unsigned char *dif_storep;	/* protection info */
+static struct sd_dif_tuple *dif_storep;	/* protection info */
 static void *map_storep;		/* provisioning map */
 
 static unsigned long map_size;
@@ -277,11 +277,6 @@ static char sdebug_proc_name[] = "scsi_debug";
 
 static struct bus_type pseudo_lld_bus;
 
-static inline sector_t dif_offset(sector_t sector)
-{
-	return sector << 3;
-}
-
 static struct device_driver sdebug_driverfs_driver = {
 	.name 		= sdebug_proc_name,
 	.bus		= &pseudo_lld_bus,
@@ -1653,7 +1648,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	start_sec = do_div(tmp_sec, sdebug_store_sectors);
 
-	sdt = (struct sd_dif_tuple *)(dif_storep + dif_offset(start_sec));
+	sdt = dif_storep + start_sec;
 
 	for (i = 0 ; i < sectors ; i++) {
 		u16 csum;
@@ -1708,16 +1703,17 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 		ei_lba++;
 	}
 
-	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
+	/* Bytes of protection data to copy into sgl */
+	resid = sectors * sizeof(*dif_storep);
 	sector = start_sec;
 
 	scsi_for_each_prot_sg(SCpnt, psgl, scsi_prot_sg_count(SCpnt), i) {
 		int len = min(psgl->length, resid);
 
 		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
-		memcpy(paddr, dif_storep + dif_offset(sector), len);
+		memcpy(paddr, dif_storep + sector, len);
 
-		sector += len >> 3;
+		sector += len / sizeof(*dif_storep);
 		if (sector >= sdebug_store_sectors) {
 			/* Force wrap */
 			tmp_sec = sector;
@@ -1891,7 +1887,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			 * correctness we need to verify each sector
 			 * before writing it to "stable" storage
 			 */
-			memcpy(dif_storep + dif_offset(sector), sdt, 8);
+			memcpy(dif_storep + sector, sdt, sizeof(*sdt));
 
 			sector++;
 
-- 
1.8.1.4

Re: [PATCH 0/3] scsi_debug: fix data integrity support

[Douglas Gilbert]

On 13-04-21 05:17 AM, Akinobu Mita wrote:
> When I tried testing the data integrity support in scsi_debug on x86_32,
> I got CONFIG_DEBUG_HIGHMEM warnings and protection errors.  This was
> triggered due to misused kmap_atomic/kunmap_atomic.
>
> And then, while I was testing the fix of the above issue with several
> combination with module parameters dix and dif, I found that doing
> 'modprobe scsi_debug dif=0 dix=1' causes kernel crash.
>
> This patch set includes these fixes and cleanup which is related to data
> integrity support.
>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
>
> Akinobu Mita (3):
>    scsi_debug: fix data integrity support on highmem machine
>    scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1
>    scsi_debug: simplify offset calculation for dif_storep
>
>   drivers/scsi/scsi_debug.c | 29 ++++++++++++-----------------
>   1 file changed, 12 insertions(+), 17 deletions(-)

Again, I'd like to see some feedback from Martin Petersen
on this set of patches. For my part, for this patch series
(1/3 to 3/3):

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

Re: [PATCH 1/3] scsi_debug: fix data integrity support on highmem machine

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

Akinobu> kmap_atomic() is now using stack based implementation and
Akinobu> doesn't take the KM_type argument anymore.  So nesting
Akinobu> kmap_atomic() calls must be properly stacked.

Akinobu> This fixes nesting kmap_atomic() calls for scsi_sglist and
Akinobu> scsi_prot_sglist in prot_verify_write().

I don't see how the prog_sglist is incorrectly nested? Also, with your
code you also end up mapping and unmapping the protection page for every
data segment. The two scatterlists have different cadence.


Akinobu> This also fixes another issue that invalid kmap address is used
Akinobu> for kunmap_atomic(): the kmap address 'daddr' is incremented in
Akinobu> the loop for each data page, and it can reach the next page
Akinobu> boundary.

That fix is fine.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 3/3] scsi_debug: simplify offset calculation for dif_storep

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

+		sector += len / sizeof(*dif_storep);

I'd rather see sizeof(struct scsi_dif_tuple) here. But that's just
personal preference. Otherwise ok.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/3] scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

Akinobu> The protection info dif_storep is allocated only when parameter
Akinobu> dif is not zero.  But it will be accessed when reading or
Akinobu> writing to the storage installed with parameter dix is not
Akinobu> zero.

Akinobu> So kernel crashes if scsi_debug module is loaded with
Akinobu> parameters dix=1 and dif=0.

The full story is that scsi_debug does not support DIF and DIX correctly
by virtue of simultaneously being the HBA and the target. And since
there is no actual data transfer between the HBA and the target the
notion of DIF is weak at best.

I did look into making scsi_debug do the right thing but it's quite a
bit of code and I lost interest about halfway through the effort. If
you'd like to fix this properly I can probably find the patch to use as
baseline?

In this particular case I'm ok with your patch keying off of
scsi_debug_dix. But it's very important to me that I'm able to set
P_TYPE and the host protection mask as I see fit using the module
parameters. Even when the combination of options don't make strict
sense.

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

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/3] scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1

[Akinobu Mita]

2013/4/25 Martin K. Petersen <martin.petersen@oracle.com>:
>>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
>
> Akinobu> The protection info dif_storep is allocated only when parameter
> Akinobu> dif is not zero.  But it will be accessed when reading or
> Akinobu> writing to the storage installed with parameter dix is not
> Akinobu> zero.
>
> Akinobu> So kernel crashes if scsi_debug module is loaded with
> Akinobu> parameters dix=1 and dif=0.
>
> The full story is that scsi_debug does not support DIF and DIX correctly
> by virtue of simultaneously being the HBA and the target. And since
> there is no actual data transfer between the HBA and the target the
> notion of DIF is weak at best.
>
> I did look into making scsi_debug do the right thing but it's quite a
> bit of code and I lost interest about halfway through the effort. If
> you'd like to fix this properly I can probably find the patch to use as
> baseline?

I'm interested in the patch.  So could you provide it?

Re: [PATCH 1/3] scsi_debug: fix data integrity support on highmem machine

[Akinobu Mita]

2013/4/25 Martin K. Petersen <martin.petersen@oracle.com>:
>>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
>
> Akinobu> kmap_atomic() is now using stack based implementation and
> Akinobu> doesn't take the KM_type argument anymore.  So nesting
> Akinobu> kmap_atomic() calls must be properly stacked.
>
> Akinobu> This fixes nesting kmap_atomic() calls for scsi_sglist and
> Akinobu> scsi_prot_sglist in prot_verify_write().
>
> I don't see how the prog_sglist is incorrectly nested? Also, with your
> code you also end up mapping and unmapping the protection page for every
> data segment. The two scatterlists have different cadence.

I meant that the unmapped address by kunmap_atomic() should be the
address returned by the last kmap_atomic() call which is not yet unmapped.
Because kmap_atomic was changed to stack based implementation.

Specifically,

    /* For each data page */
    scsi_for_each_sg(SCpnt, dsgl, scsi_sg_count(SCpnt), i) {
        daddr = kmap_atomic(sg_page(dsgl)) + dsgl->offset;

This kmapped address 'daddr' will not unmapped during the next for-loop.

        /* For each sector-sized chunk in data page */
        for (j = 0 ; j < dsgl->length ; j += scsi_debug_sector_size) {

            /* If we're at the end of the current
             * protection page advance to the next one
             */
            if (ppage_offset >= psgl->length) {
                kunmap_atomic(paddr);
                ...

But this kunmap_atomic() for the first time in this loop is called.
The last two kmap_atomic() and kunmap_atomic() do not correspond and
it breaks kmap_atomic/kunmap_atomic stack.