Skip to contentSkip to conversation

mpt3sas: Limit NVMe request size to 2 MiB

PATCH Superseded Ranjan Kumar 2 participants 4 messages Apr 11 – Apr 13

Subsystems (3)
Stable branch SCSI LSI Logic MPT fusion drivers (FC/SAS/SPI)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Ranjan Kumar

author maintainer
#0 lore raw

PATCH v2 1/1 mpt3sas: Limit NVMe request size to 2 MiB

Some firmware reports NVMe maximum transfer sizes that follow the drive capability. When those values are very large, the block layer may build I/O that this driver cannot handle, which can cause a kernel oops.

When an NVMe device is set up, cap how large a single transfer may be to the smaller of the firmware-reported limit and roughly two mebibytes with a small margin. If no valid limit is reported, apply the same upper bound.

Cc [email protected]
Fixes 9b8b84879d4a ("block: Increase BLK_DEF_MAX_SECTORS_CAP")
Reported-by Mira Limbeck <[email protected]>
Closes https://lore.kernel.org/r/[email protected] open here
Link https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b8b84879d4a
Suggested-by Keith Busch <[email protected]>
Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_scsih.c +11 -2
index 6ff788557294..fca9d6722fc8 100644
@@ -54,6 +54,7 @@
 #include <linux/interrupt.h>
 #include <linux/raid_class.h>
 #include <linux/unaligned.h>
+#include <linux/sizes.h>
 
 #include "mpt3sas_base.h"
 

@@ -2737,9 +2738,17 @@ scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim)
 				"connector name( %s)\n", ds,
 				pcie_device->enclosure_level,
 				pcie_device->connector_name);
-
+		/*
+		 * Firmware may report large NVMe MDTS values on some ASICs.
+		 * Limit max_hw_sectors to the smaller of the reported MDTS
+		 * and 2 MiB to avoid issuing I/O the driver cannot handle.
+		 */
 		if (pcie_device->nvme_mdts)
-			lim->max_hw_sectors = pcie_device->nvme_mdts / 512;
+			lim->max_hw_sectors = min_t(u32,
+					pcie_device->nvme_mdts / 512,
+					(SZ_2M / 512));
+		else
+			lim->max_hw_sectors = (SZ_2M / 512);
 
 		pcie_device_put(pcie_device);
 		spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);
Signature · 1 line
2.47.3
author maintainer
#1 lore raw

PATCH v2 1/1 mpt3sas: Limit NVMe request size to 2 MiB

Some firmware reports NVMe maximum transfer sizes that follow the drive capability. When those values are very large, the block layer may build I/O that this driver cannot handle, which can cause a kernel oops.

When an NVMe device is set up, cap how large a single transfer may be to the smaller of the firmware-reported limit and roughly two mebibytes with a small margin. If no valid limit is reported, apply the same upper bound.

Cc [email protected]
Fixes 9b8b84879d4a ("block: Increase BLK_DEF_MAX_SECTORS_CAP")
Reported-by Mira Limbeck <[email protected]>
Closes https://lore.kernel.org/r/[email protected] open here
Link https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b8b84879d4a
Suggested-by Keith Busch <[email protected]>
Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_scsih.c +11 -2
index 6ff788557294..fca9d6722fc8 100644
@@ -54,6 +54,7 @@
 #include <linux/interrupt.h>
 #include <linux/raid_class.h>
 #include <linux/unaligned.h>
+#include <linux/sizes.h>
 
 #include "mpt3sas_base.h"
 

@@ -2737,9 +2738,17 @@ scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim)
 				"connector name( %s)\n", ds,
 				pcie_device->enclosure_level,
 				pcie_device->connector_name);
-
+		/*
+		 * Firmware may report large NVMe MDTS values on some ASICs.
+		 * Limit max_hw_sectors to the smaller of the reported MDTS
+		 * and 2 MiB to avoid issuing I/O the driver cannot handle.
+		 */
 		if (pcie_device->nvme_mdts)
-			lim->max_hw_sectors = pcie_device->nvme_mdts / 512;
+			lim->max_hw_sectors = min_t(u32,
+					pcie_device->nvme_mdts / 512,
+					(SZ_2M / 512));
+		else
+			lim->max_hw_sectors = (SZ_2M / 512);
 
 		pcie_device_put(pcie_device);
 		spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);
Signature · 1 line
2.47.3
#2 lore raw
Quoted 8 lines · Ranjan Kumar

Some firmware reports NVMe maximum transfer sizes that follow the drive capability. When those values are very large, the block layer may build I/O that this driver cannot handle, which can cause a kernel oops.

When an NVMe device is set up, cap how large a single transfer may be to the smaller of the firmware-reported limit and roughly two mebibytes with a small margin. If no valid limit is reported, apply the same upper bound.

What margin ? I do not see any...

Quoted 25 lines
Cc [email protected]
Fixes 9b8b84879d4a ("block: Increase BLK_DEF_MAX_SECTORS_CAP")
Reported-by Mira Limbeck <[email protected]>
Closes https://lore.kernel.org/r/[email protected] open here
Link https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b8b84879d4a
Suggested-by Keith Busch <[email protected]>
Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -0
index 6ff788557294..fca9d6722fc8 100644
@@ -54,6 +54,7 @@
 #include <linux/interrupt.h>
 #include <linux/raid_class.h>
 #include <linux/unaligned.h>
+#include <linux/sizes.h>
 
 #include "mpt3sas_base.h"

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2740

Quoted 4 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +0 -1
@@ -2737,9 +2738,17 @@ scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim)
 				"connector name( %s)\n", ds,
 				pcie_device->enclosure_level,
 				pcie_device->connector_name);
-

Spurious whiteline change. The white line is nice before the big block below.

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2742

Quoted 2 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +2 -0
+		/*
+		 * Firmware may report large NVMe MDTS values on some ASICs.

What ASICs ? The SSD controller or the HBA controller ? Also, does the HBA firmware change the MDTS ? Or does it report the SSD reported MDTS as is ? If it is the former, then an explanation would be nice. If it is the latter, instead of "Firmware may report" I suggest "The NVMe device controller may report"

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2744

Quoted 2 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +2 -0
+		 * Limit max_hw_sectors to the smaller of the reported MDTS
+		 * and 2 MiB to avoid issuing I/O the driver cannot handle.

Without any explanations, 2MiB appears to be a "magic" value here. There is a clear explanation for it with the 4K device page size that can fit 512 PRP entries each pointing to one 4K page. So let's state that.

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2751

Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +6 -1
+		 */
 		if (pcie_device->nvme_mdts)
-			lim->max_hw_sectors = pcie_device->nvme_mdts / 512;
+			lim->max_hw_sectors = min_t(u32,
+					pcie_device->nvme_mdts / 512,
+					(SZ_2M / 512));
+		else
+			lim->max_hw_sectors = (SZ_2M / 512);

		lim->max_hw_sectors = SZ_2M >> SECTOR_SHIFT;
		if (pcie_device->nvme_mdts)
			lim->max_hw_sectors = min_t(u32, lim->max_hw_sectors,
					pcie_device->nvme_mdts >> SECTOR_SHIFT);

is I think a bit nicer.

Quoted 3 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +0 -0
 		pcie_device_put(pcie_device);
 		spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#3 lore raw

Hi Damien,

Thanks for the feedback. I will send a v3 patch addressing all issues.

Thanks,
Ranjan

Quoted 95 lines · Damien Le Moal
Quoted 8 lines · Ranjan Kumar

Some firmware reports NVMe maximum transfer sizes that follow the drive capability. When those values are very large, the block layer may build I/O that this driver cannot handle, which can cause a kernel oops.

When an NVMe device is set up, cap how large a single transfer may be to the smaller of the firmware-reported limit and roughly two mebibytes with a small margin. If no valid limit is reported, apply the same upper bound.

What margin ? I do not see any...

Quoted 5 lines
Cc [email protected]
Fixes 9b8b84879d4a ("block: Increase BLK_DEF_MAX_SECTORS_CAP")
Reported-by Mira Limbeck <[email protected]>

Closes:

https://lore.kernel.org/r/[email protected]

Quoted 1 line

Link:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b8b84879d4a

Review

Quoted 7 lines
Suggested-by Keith Busch <[email protected]>
Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev,

Quoted 12 lines

index 6ff788557294..fca9d6722fc8 100644

drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -0
@@ -54,6 +54,7 @@
 #include <linux/interrupt.h>
 #include <linux/raid_class.h>
 #include <linux/unaligned.h>
+#include <linux/sizes.h>
 
 #include "mpt3sas_base.h"
 

@@ -2737,9 +2738,17 @@ scsih_sdev_configure(struct scsi_device *sdev,

struct queue_limits *lim)

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2740

Quoted 4 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +0 -1
                              "connector name( %s)\n", ds,
                              pcie_device->enclosure_level,
                              pcie_device->connector_name);
-

Spurious whiteline change. The white line is nice before the big block below.

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, · line 2742

Quoted 2 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +2 -0
+             /*
+              * Firmware may report large NVMe MDTS values on some

ASICs.

What ASICs ? The SSD controller or the HBA controller ? Also, does the HBA firmware change the MDTS ? Or does it report the SSD reported MDTS as is ? If it
is the former, then an explanation would be nice. If it is the latter, instead
of "Firmware may report" I suggest "The NVMe device controller may report"

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2744

Quoted 2 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +2 -0
+              * Limit max_hw_sectors to the smaller of the reported MDTS
+              * and 2 MiB to avoid issuing I/O the driver cannot handle.

Without any explanations, 2MiB appears to be a "magic" value here. There is a
clear explanation for it with the 4K device page size that can fit 512 PRP entries each pointing to one 4K page. So let's state that.

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2751

Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +6 -1
+              */
              if (pcie_device->nvme_mdts)
-                     lim->max_hw_sectors = pcie_device->nvme_mdts / 512;
+                     lim->max_hw_sectors = min_t(u32,
+                                     pcie_device->nvme_mdts / 512,
+                                     (SZ_2M / 512));
+             else
+                     lim->max_hw_sectors = (SZ_2M / 512);

                lim->max_hw_sectors = SZ_2M >> SECTOR_SHIFT;
                if (pcie_device->nvme_mdts)
                        lim->max_hw_sectors = min_t(u32,
lim->max_hw_sectors,
                                        pcie_device->nvme_mdts >>

SECTOR_SHIFT);

is I think a bit nicer.

drivers/scsi/mpt3sas/mpt3sas_scsih.c scsih_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim) · line 2754

Quoted 3 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +0 -0
              pcie_device_put(pcie_device);
              spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);

--
Damien Le Moal
Western Digital Research